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

Instances with color? #234

Closed
emilk opened this issue Apr 20, 2022 · 9 comments
Closed

Instances with color? #234

emilk opened this issue Apr 20, 2022 · 9 comments
Labels

Comments

@emilk
Copy link
Contributor

emilk commented Apr 20, 2022

Currently three_d::Instance has a geometry_transform and a texture_transform. I would be interested in extending that with color (albedo).

This sparks two questions:

  • What other attributes might we want to add?
  • How can we avoid having to pay for unused attributes? (already I'm paying for texture_transform which I'm not using; adding albedo will increase the cost for those not using that)

One solution is to replace Vec<Instance> with a SoA solution:

pub struct InstancesAttributes {
    pub geometry_transforms: Vec<Mat4>,
    pub texture_transform: Option<Vec<Mat4>>,
    pub albedos: Option<Vec<Mat4>>,}

Of course this increases the complexity of the implementation.

Anyway, I just wanted to open the discussion.

@asny
Copy link
Owner

asny commented Apr 20, 2022

Good point (as always 🙂 ). This is basically the same problem as the vertex buffers for Mesh and InstancedMesh. My idea was that it should be simple to implement your own mesh type with your own custom vertex and instance attributes. And it is not a lot of work; if you know exactly which attributes you have, half of the code in Mesh and InstancedMesh can be avoided. However, I guess people will not often do it unless they really have to. So the way it is solved for vertex attributes is by making most attributes optional, as you are also suggesting. Maybe that's just the solution. I will think about it 💪 other suggestions are more than welcome 👍

@asny
Copy link
Owner

asny commented Apr 30, 2022

@emilk Could something like #239 work? Specifically look at the Instances struct.
And how exactly should the colors work? Are they multiplied onto per vertex colors or can be used without per vertex colors?

@emilk
Copy link
Contributor Author

emilk commented May 1, 2022

Looks good! I can see if I can test it soemtimes this week.

I think the most powerful would be if the instance colors were multiplied with the existing vertex (and texture) colors. That would allow users to tint meshes and/or make some of them more or less transparent.

@asny
Copy link
Owner

asny commented May 2, 2022

No worries, I will also make a test example (also to test/showcase per instance texture transforms), just wanted to make sure it's in the right direction 🙂

I agree, when this is done, the albedo will be calculated by

albedo = per-vertex color x per-instance color x material albedo x material albedo texture

This is assuming the material support per-vertex colors (which most of them does). However, right now, the per-instance color is only used if the per-vertex colors are specified, which, now I write it, doesn't really make sense. So I will change it such that you can specify either the per-vertex color or the per-instance color or both 💪

@asny asny added the 0.12 label May 5, 2022
@asny
Copy link
Owner

asny commented May 12, 2022

I have merged the changes (#239), so now per instance colors are supported and they can also be combined with per vertex colors 💪 I haven't made a test case (but I did test it) because I think a point cloud is a nice test case so the test case will be part of #242.

In addition, I want to split the geometry_transformation into a scale, rotation and position part, so not closing this issue just yet. Right now each instance uses 3 vec4s but usually you just want to set a position and can do with one vec3 per instance. Worst case, the memory usage will be the same though.

@asny
Copy link
Owner

asny commented May 19, 2022

I split the transformation into position, rotation and scale so I'm going to close this issue. @emilk hope it solved your problem 🙂

@asny asny closed this as completed May 19, 2022
@emilk
Copy link
Contributor Author

emilk commented May 21, 2022

I finally got around to try out the new Instances - they work great! I found two gotchas.

The first: Instances::default is not empty (#250).

The second: I started out creating an InstancedModel with just Instanced::default() as the argument, to later call set_instances. However, that did not work. It took me a while to figure out I had to pre-declare what instance attributes that I was gonna need when creating the InstancedModel. This makes sense, but wasn't documented, and using it the wrong way just lead to a silent error. I would prefer a log line, an Error or even a panic to silently accepting my bad behavior!

Otherwise: works great, thanks so much!

@asny
Copy link
Owner

asny commented May 21, 2022

No problem 🙂 and thanks for the feedback 👍

I will merge #250 soon and about the second problem, I think it should be possible to avoid errors in that case, but I'll need to take a closer look before I am certain. In any case I agree that it should not be a silent error, I always try to avoid that but can sometimes be difficult when working with graphics 🙂

@asny asny reopened this May 21, 2022
asny added a commit that referenced this issue May 22, 2022
@asny
Copy link
Owner

asny commented May 22, 2022

Yeah, the second problem was just a mistake from my side, I have fixed both issues 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants