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

Separated to two constructors #346

Conversation

jacky-ttt
Copy link
Contributor

Separated BaseGeometryNode to two constructors. One with materialInstance that binds a material instance to all primitives, while the other with materialInstances that binds one material instance to each face.

Copy link
Contributor

@ThomasGorisse ThomasGorisse left a comment

Choose a reason for hiding this comment

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

I would have been great to have a singlePrimitive : Boolean parameter on Geometry so we can apply the same single material to all the Geometrybut it's a good start. For example, in case on textured one we want to apply all around the geometry without repeating it.
Could you please just make the material optional (nullable) so we don't apply any if not required?
Thanks

@@ -25,7 +21,7 @@ open class CubeNode(
* Should return the material to bind for the zero-based index of the primitive, must be less
* than the [Geometry.submeshes] size passed to constructor.
*/
materialInstances: (index: Int) -> MaterialInstance? = { materialInstance },
materialInstances: (index: Int) -> MaterialInstance,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why moving it to non nullable?
It could be usefull in case only some faces requiring material.
Same for every Geometries they could be used for collision purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought a face must be attached with a material. It is fixed now.

/**
* Binds a material instance to all primitives.
*/
materialInstance: MaterialInstance,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also be nullable for every geometries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@jacky-ttt
Copy link
Contributor Author

I would have been great to have a singlePrimitive : Boolean parameter on Geometry so we can apply the same single material to all the Geometrybut it's a good start. For example, in case on textured one we want to apply all around the geometry without repeating it. Could you please just make the material optional (nullable) so we don't apply any if not required? Thanks

I don't think singlePrimitive : Boolean is needed. You can just assign a value to materialInstance in the second constructor so that all faces are applied with the same material.

@ThomasGorisse
Copy link
Contributor

You're right but if we take the image texture example, actually, if we want to apply it to let say a Cube, we will have the same image displayed on each face and no way to have the same single image applied all around the Cube (each face displaying a part of the image).

@ThomasGorisse ThomasGorisse merged commit dbb244c into SceneView:main Nov 1, 2023
1 check passed
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