Skip to content

Conversation

@Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Nov 22, 2020

Related issue: Follow-up of #17745.

Description

During testing in the editor, I've overlooked a use case in #17745. When a JSON only contains data textures and no instances of HTMLImageElement, onLoad() is currently not fired. That happens because of this line:

if ( json.images === undefined || json.images.length === 0 ) {

The loader calls no onLoad() since it will be called when loading images via the internal ImageLoader. However, data textures don't use image loader which means ObjectLoader.parse() needs a different check.

@mrdoob
Copy link
Owner

mrdoob commented Nov 22, 2020

I think this achieves the same?

	if ( onLoad !== undefined ) {

		for ( const uuid in images ) {

			if ( images[ uuid ] instanceof HTMLImageElement ) {

				onLoad( object );
				break;

			}

		}

	}

@mrdoob mrdoob added this to the r123 milestone Nov 22, 2020
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 22, 2020

The loader should fire onLoad() at this place if there are no HTMLImageElements. It seems your code fires onLoad() as soon as an image element is in the images array.

@mrdoob
Copy link
Owner

mrdoob commented Nov 22, 2020

Ah, I see... Then this?

	if ( onLoad !== undefined ) {

		let hasImages = false;

		for ( const uuid in images ) {

			if ( images[ uuid ] instanceof HTMLImageElement ) {

				hasImages = true;
				break;

			}

		}

		if ( hasImages === false ) onLoad( object );

	}

Basically trying to avoid doing any work if onLoad is not defined.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 22, 2020

Basically trying to avoid doing any work if onLoad is not defined.

Makes sense! Yes, this approach works 👍 .

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 22, 2020

Okay, updated the PR!

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 22, 2020

The editor can now save/reload scenes with HDR textures 🎉 .

image

@mrdoob mrdoob merged commit c37b1e7 into mrdoob:dev Nov 22, 2020
@mrdoob
Copy link
Owner

mrdoob commented Nov 22, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants