Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editor: Fixed texture load issues. #9487

Merged
merged 2 commits into from Aug 18, 2016
Merged

Editor: Fixed texture load issues. #9487

merged 2 commits into from Aug 18, 2016

Conversation

tschw
Copy link
Contributor

@tschw tschw commented Aug 10, 2016

Apparently, the file input element won't fire a change event unless reset. Because of this, the editor won't allow loading the same texture after it has been loaded before, which makes the control irresponsive in some cases.

Reproduction (Chrome):

  • Load a model
  • Load a texture, but don't enable it
  • Add a light source
  • Switch back to the model

At this point the texture is forgotten (which is fine, since it's only remembered in the GUI components) but it can't be reloaded because the invisible file input control won't see the change.

This patch adds a form element around the file input control to properly reset it when an image has been loaded. This way, the same image can be loaded multiple times. It also prevents textures that don't exist from being enabled in the editor, because it doesn't make sense.

@tschw
Copy link
Contributor Author

tschw commented Aug 10, 2016

@mrdoob Would it be possible to also patch the online version, maybe? Would be super cool, then wouldn't have to host a patched copy on the LAN to make our artists happy...

@@ -58,6 +63,7 @@ UI.Texture = function ( mapping ) {

var texture = new THREE.CanvasTexture( canvas, mapping );
texture.sourceFile = file.name;
form.reset();
Copy link
Owner

@mrdoob mrdoob Aug 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried doing input.value = '' instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See below.

@mrdoob
Copy link
Owner

mrdoob commented Aug 11, 2016

Would be super cool, then wouldn't have to host a patched copy on the LAN to make our artists happy...

"out artists"... where do you work? 😁

@tschw
Copy link
Contributor Author

tschw commented Aug 11, 2016

Reads "our" - not "autists" :).

Trying to change the state of the input element directly would be risky, since browsers usually forbid it to keep malicious sites from snooping on sensitive local files.

@@ -893,6 +893,8 @@ Sidebar.Material = function ( editor ) {

}

signals.materialChanged.dispatch( material );
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SetMaterialValueCommand already dispatches materialChanged. So this would make it dispatch twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, better. Still needs to run updateUI to keep null maps from being enabled, though.

@mrdoob
Copy link
Owner

mrdoob commented Aug 18, 2016

Looks good now!

@mrdoob mrdoob merged commit b60af57 into mrdoob:dev Aug 18, 2016
@mrdoob
Copy link
Owner

mrdoob commented Aug 18, 2016

Thanks!

aardgoose pushed a commit to aardgoose/three.js that referenced this pull request Oct 7, 2016
* Editor: Fixed texture load issues.

* Don't update twice, but forbid enabling empty maps.
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.

None yet

2 participants