Skip to content

Conversation

@polygontwist
Copy link
Contributor

Description

For the itentification of the element the name is helpful.

VRML: DEF ring Transform ->"ring"

geometry.name=node.DEF;

for buildIndexedFaceSetNode
for buildGroupingNode

For the itentification of the element the name is helpful.

VRML: DEF ring Transform ->"ring"
geometry.name=node.DEF; 
for buildIndexedFaceSetNode
for buildGroupingNode
@Mugen87 Mugen87 changed the base branch from master to dev February 5, 2021 18:51
@Mugen87 Mugen87 changed the title adding Names of Nodes VRMLLoader: Adding names of nodes. Feb 5, 2021
@Mugen87 Mugen87 self-requested a review February 5, 2021 18:51

}

object.name=node.DEF;
Copy link
Collaborator

@Mugen87 Mugen87 Feb 5, 2021

Choose a reason for hiding this comment

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

Please note that DEF is optional so it can be undefined and thus Object3D.name. This is not good.

I suggest you change the lines to this pattern:

if ( node.DEF !== undefined ) object.name = node.DEF;

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.
Is there anything else I need to do here - I'm not that familiar with github?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The idea is that you push an additional commit with the requested changes.

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe he did the PR from the website?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@polygontwist If okay for you, I can do this change tomorrow. Otherwise I suggest you fork the repository, applying your changes in a new branch and then make a PR. So the typical contribution workflow.

Copy link
Contributor Author

@polygontwist polygontwist Feb 7, 2021

Choose a reason for hiding this comment

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

@mrdoob: yes, from this website.

@Mugen87: yes, please make it so.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 5, 2021

Please change the file examples/js/loaders/VRMLLoader.js and then generate the module via node utils/modularize.js.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 5, 2021

I think it makes sense to enhance buildAppearanceNode() and set the name for the material if available.

@mrdoob mrdoob added this to the r126 milestone Feb 5, 2021
@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 7, 2021

Closing in favor of #21217.

@Mugen87 Mugen87 closed this Feb 7, 2021
@Mugen87 Mugen87 removed this from the r126 milestone Feb 7, 2021
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.

3 participants