Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Entity Color Options #1005
base: main
Are you sure you want to change the base?
Entity Color Options #1005
Changes from 5 commits
22389ca
61a0703
9221018
66c86e9
cbdabf0
cb37ee5
b28fe73
6a4f778
6b3c965
bf3c468
70efd63
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Properly unregister this model-loaded listener in remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the
this.updateMaterials = this.updateMaterials.bind(this);
at the beginning. Listening to model-loaded shouldn't be needed anymore if you listen on object3dset. object3dset is fired just before model-loaded https://github.com/aframevr/aframe/blob/cc43ea67f8170d8ca7e5bdcbaade031df103801e/src/components/gltf-model.js#L48-L49There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove all this and use
after you redefined the custom-colors schema to use the custom parse/stringify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to serialize yourself, just give the object to execute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be done in the aframe component, not here. There is probably an issue here, it will override material.userData.origColor with the current customized color if you select another car and back this car because the callback will be recreated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I changed it to be initialized once whenever the component is added. Since this presents an issue if the model is changed, I will make it so that changing the model will remove the
custom-colors
component. This makes most sense, as most models will have unique materialsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add
{once: true}
for this listener.