-
Notifications
You must be signed in to change notification settings - Fork 34
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
[WIP] Adding color options to entities #992
Conversation
src/components/accent-color.js
Outdated
this.meshes = []; | ||
// The group children aren't ready when the component is initialized, so wait a bit. | ||
// TODO: Find a better way to time this. | ||
setTimeout(() => { |
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 is a workaround since the entity children aren't visible when the component is initialized. I'm curious which event is fired that can be used to time this correctly.
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.
gltf-model
component fires a model-loaded
event, so you probably want to listen to that.
if (this.el.getObject3D('mesh')) {
// do your thing
} else {
this.el.addEventListener('model-loaded', () => {
this.meshes.length = 0; // be sure to not keep previous gtlf meshes if we switched the model
// do your thing
})`
}
I wouldn't keep meshes in an array on the component, that's source of issues, for example if you remove the gtlf-model component, you currently keep meshes in the array that aren't garbage collected. I would just do the traverse in update, without storing an array of meshes on the component.
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.
Sounds good, I tried the model-loaded
and it works perfect!
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.
The this.el.getObject3D('mesh')
check is needed if the accent-color component is added later, so after you got that model-loaded event. gltf-model set the loaded glb with el.setObject3D('mesh', model)
'accent-color': { | ||
index: -1 | ||
} | ||
} |
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.
I wouldn't add the component by default, that's not needed unless you use it. I would create a button in the Sidebar for example "customize color" (for mixins that makes sense) that adds the component to the entity. You can do that with AFRAME.INSPECTOR.execute('componentadd', {entity: entity, component: 'accent-color', value: ''})
const mesh = meshes.at(this.data.index); | ||
if (mesh) { | ||
mesh.material.color.set(this.data.color); | ||
mesh.material.color.multiplyScalar(255); |
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.
Why do you multiply by 255?
In the UI, the index field is not really understandable by the user. I would dynamically get all material names of the model and show a select here. Not sure what are the names currently in the models, it may be nice to rename them in each model to have something clean. |
|
You're both talking about mesh names. I was talking about material names. Some models can have several meshes that uses the same material, so that doesn't make sense to me to chose a mesh. Tools like gltf-transform lose the mesh names, but keep the material name if you add |
Thanks for the clarification Vincent. It may make sense to have the original artist help with the conversion, perhaps we could put a list together of which files and what are the modifications we want. I can then get a quote and see if it's something we can afford in the short term or if we should do it ourselves. Unrelated but part of the same files, we also discussed earlier removing rigging for wheels and instead using named meshes for wheel to allow rotation when animating. (rigging was overkill) |
Another idea @ewei2406 , the Isuzu truck model in blender appears to have the named material "vehicle-body" already in a way that we might want. Perhaps you could experiment with exporting in compressed glb but in a way that retains that material name so we can target it in the color component. If it can work in this one model then we can apply the same changes to other vehicle models? |
Sounds good, I will try to draft up what is discussed. I will take a look at using blender to modify the meshes - I was able to transform the |
Hi @ewei2406 |
See #1005 |
Based on #166, I was playing around with options for adding color. Here's a basic implementation of a simple approach to just modify the contents of meshes
object3D
to have acolor
property on their materials.index
field so that the user can specify which mesh to change the color for (though this requires a bit of guessing, and leave it to-1
for no color applied).accent-color
component which has thecolor
andindex
values, and is grouped under the "advanced" section.color
field doesn't really work as a true color change but more of a tint. So it isn't exactly ideal, but it works for all entities without any changes to the model files.