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

Default material color for 3mf loader #22801

Closed
wants to merge 3 commits into from

Conversation

kovacsv
Copy link
Contributor

@kovacsv kovacsv commented Nov 9, 2021

3mf files often come without material definitions. In this case the default material color was gratuitously purple. I've added the possibility to set this default color from the outside.

3mf files often come without material definitions. In this case the default material color was gratuitously purple. I've added the possibility to set this default color from the outside.
@@ -47,9 +47,16 @@ class ThreeMFLoader extends Loader {
super( manager );

this.availableExtensions = [];
this.defaultMaterialColor = 0xaaaaff;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we define this as a THREE.Color object? setDefaultMaterialColor() would be defined like so then:

this.defaultMaterialColor.set( defaultMaterialColor );

It's more consistent if color properties are actually of type THREE.Color like with materials.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I've modified it to store a THREE.Color object.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 9, 2021

Um, in general we do not provide loader interfaces that allow to configure the default material color or material type etc. If we start with 3MFLoader, we potentially have to do this for other loaders, too. And this might blow up the current simple loader API.

It's actually no big deal to change the material color on app level after the loading process. Introducing setDefaultMaterialColor() would just be a convenient method.

@mrdoob What is your preference?

@kovacsv
Copy link
Contributor Author

kovacsv commented Nov 9, 2021

Just wanted to add, that there are two types of loader parameters:

  1. Loader independent, like setDefaultMaterialColor. It can be utilized by almost all loaders, and could be placed in the Loader base class, although it can be confusing if not all loaders utilize this value.
  2. Loader specific parameters, like turning off the huge sphere generation for sky in the VRMLLoader. This one probably should be loader dependent.

@kovacsv
Copy link
Contributor Author

kovacsv commented Nov 9, 2021

It's actually no big deal to change the material color on app level after the loading process.

Yes, if you know which materials are generated by the loader, and which came from the model. Usually it's not easy to tell. Even if you understand the internals of the loader you can't decide if it's the default material, or a material with the same color that is coming from the model.

@gkjohnson
Copy link
Collaborator

Is there a good reason for the default color to be 0xaaaaff? I would just expect the default color to be white like other loaders do (OBJLoader, for example).

@mrdoob
Copy link
Owner

mrdoob commented Nov 10, 2021

I agree. I think it would be better to just make 0xffffff the default.

@kovacsv Would you still need something like setDefaultMaterialColor() in that case?

@kovacsv
Copy link
Contributor Author

kovacsv commented Nov 10, 2021

@mrdoob On the long term I'd be happy to see a common interface for all loaders to set the default material, because in my use case users are able to modify the material if the imported model doesn't contain any material information.

On the other hand I understand that this is a bigger task, and for now changing the 3mf default color to white seems better than the purple one, so I'll open another pull request to do that.

Thanks for the help and the input, I close this one now.

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.

4 participants