Add comments about default gravity in the particle system.

[iwanders]

Hi again, let me start this PR by saying; Thanks for this library, it's been a joy to work with so far!

Today I ran into the first surprise using it: I was trying to integrate the fireworks example into my own project; copied in some parts, modified the origin, scaling etc... then I noticed all particles were flying to the side by a few meters during the explosion time. Looking at the velocity generation from the example, I couldn't figure out how that could be the case...

After a while I discovered that there's a default gravity of (0.0, -9.82, 0.0) in the ParticleSystem. But that default doesn't work for me: I'm a roboticist and used to a coordinate system with x forward, y to the side and z being upwards. To be able to do all math and rendering in a familiar coordinate system my camera has an up vector of (0.0, 0.0, 1.0), which meant that the default gravity of the particles makes everything fly off in the y direction.

This PR adds a bunch of comments throughout the fireworks example, including calling out the default gravity, currently that's not mentioned in the example. It also adds it to the main section of the ParticleSystem documentation. Currently the default acceleration is mentioned for the acceleration field, but not in the main section. I think this is worth calling out, I at least found it surprising.

I'd even consider changing the default acceleration to be zero, happy to make that change, but thought that improving comments and docs would go a long way while being backwards compatible.

Edit: Discovered that to hide particles that are behind other geometries in my scene I had to set the depth_test RenderStates attribute to LessOrEqual. In the example that is set to Always, to me the example looks the same with that set to DepthTest::LessOrEqual. The pipeline what looks like a flakey error retrieving a dependency, so going ahead and pushing a commit to change that to DepthTest::Always.

[asny]

Hi and thanks for the kind words, I really appreciate it 🙏

I think you are right, it’s a bit weird that there is a default acceleration. I have changed it so you’ll have to pass in an acceleration in Particles::new. And I appreciate the concern about backwards compatibility, it’s a fine line between preserving backwards compatibility and keep improving the crate (at least before the 1.0.0 release). In this case, I think it’s a clear improvement.

About the DepthTest, I can’t see a difference either, and I can’t remember why I set it like that. It’s a really old example. Might be because it makes a difference if also writing to depth (if the write mask is COLOR_AND_DEPTH).

Thanks for the contributions 💪

[iwanders]

I have changed it so you’ll have to pass in an acceleration in Particles::new. [...] In this case, I think it’s a clear improvement.

Oh, that's an even better solution, agreed. Thanks for doing that.

I appreciate the concern about backwards compatibility, it’s a fine line between preserving backwards compatibility and keep improving the crate (at least before the 1.0.0 release).

Check, I wasn't quite sure about the backwards compatibility policy. With that in mind, some more thoughts for consideration :)

The use of cgmath::Matrix4 throughout this crate is a great choice. When I was converting my visualizer over from individual CpuMeshes everywhere to Instances, I was surprised that Instances itself doesn't have an pub transforms Option<Vec<Mat4>> field. I had standard 4x4 transformation matrices for all my instances, but now had to break up that matrix to be able to use the instancing and then the rotations field takes a Quaternion, but there's no easy way in cgmath to convert the top left 3x3 rotation matrix of the Mat4 (if there's no scaling) into a quaternion, so I ended up writing ToQuaternion. In the actual instance update code these translations, scales and rotations are all multiplied with each other (or identity), so being able to provide a Mat4 would also allow performance improvement at runtime. The overall performance difference between instanced and non instanced was very profound for my use case, way more than I expected. It may even be worth making an example for this where we have like 100 3d sphere's flying around randomly and a toggle in the UI to toggle between instanced and non instanced. Happy to try to take a stab at this example and/or the Mat4 support in Instances if you agree.

The second thing is - again because of familiarity with other tools - that I wanted to be able to pan the OrbitControl, so change the point it rotates around. With the current OrbitControl that's not really possible, so I ended up making these changes (not sure if the math is 100% right, but it works well enough), effectively using the camera.target() instead of embedding it in the CameraControl's callbacks. I also had to be able to change the speed, on Linux the current orbit control was jumping in very large steps. With these changes I could now instantiate it with a config struct to achieve what I wanted. I expected these changes to be too significant, but happy to file a PR and iterate on it if you'd like to incorporate them.

[asny]

Sorry for the late reply, I'm trying to take a small break from my computer - it's not going super great 🙂

Really nice feedback, I really appreciate it! It can be hard for me to see where there can be usability problems since I'm so familiar with everything, so please keep the feedback coming if you have any 🙂 And if you want to make some example, please go ahead, I think the more examples the better! It's a great way of contributing!

I see the problem with instances having rotations, translations and scale instead of just a transformation matrix, maybe that was not a great choice. My reason for doing it, is that if you only want to apply a rotation to each instance, you still need 12 floats on the GPU for each instance (the first three rows of a Mat4) to represent that rotation. Instead you could do with just a 4 floats (a quaternion). However, at the moment, the implementation still uses a Mat4 for each instance, I didn't have time to do the actual optimisation. I'm not even sure how much it matters, it very much depends on which types of transformations (translation, rotation, scale) you want to apply and how many instances you have. However, cgmath implements conversion between Mat4 and Quat so it should hopefully be relatively easy to convert. Anyway, let me just think about it before we do a lot of changes 🙂 Could for example be nice to be able to provide both Mat4 or rotation, translation and scale, but I'm not sure it's worth the effort?

Nice to hear that you got a speedup using instances, it is a really powerful optimisation 👍 And as I said, if you want to make an instanced example, please go ahead. The wireframe example already use instances, but that might not be very clear from the name, so might be better to make a new one.

About the Controls, they are not really the best, I haven't had the time to improve them in a while and I wasn't really satisfied with the initial solution, so they can definitely use a restructuring. If you want, you are more than welcome to give it a go. I think the configuration struct is a great choice and very much in line with the rest of the crate.

[iwanders]

I've opened a PR for the proposed example, one last thing I'd like to respond to from this thread.

My reason for doing it, is that if you only want to apply a rotation to each instance, you still need 12 floats on the GPU for each instance (the first three rows of a Mat4) to represent that rotation. Instead you could do with just a 4 floats (a quaternion). However, at the moment, the implementation still uses a Mat4 for each instance, I didn't have time to do the actual optimisation. I'm not even sure how much it matters, it very much depends on which types of transformations (translation, rotation, scale) you want to apply and how many instances you have.

Yeah, I don't think that optimisation really matters, the amount of 'stuff' happening around the calculation of the updated pose / translation likely vastly exceeds the difference between sending a 4x4 or sending a quaternion. With the current API the only thing one can do is send translation and rotation, since translation is not optional. I guess it could make a difference if the gpu was used stateful, and we could solely update the rotations, but keep the positions as they were previously. (Not sure if that's possible, my understanding of modern graphics things is super limited). Personally, I'd lean towards making position optional as well and providing an optional Mat4 entry, it may seem over the top, but extending the interface here is relatively cheap.

However, cgmath implements conversion between Mat4 and Quat so it should hopefully be relatively easy to convert. Anyway, let me just think about it before we do a lot of changes slightly_smiling_face Could for example be nice to be able to provide both Mat4 or rotation, translation and scale, but I'm not sure it's worth the effort?

It implements Quaternion to Matrix3 and Matrix4, it does not provide Matrix4 to Quaternion, to do that you first have to go to Matrix3, then you can convert that to a quaternion. But I didn't see a convenient way to convert from a Matrix4 to Matrix3. So I ended (comment in the code links to an upstream issue around this) up truncating the columns, creating a new matrix and then converting that into a quaternion. Not a big deal for the developer, or for the performance, but something I ran into that was surpsising. Definitely minor in the grand scheme of things 👍

[asny]

By optimisation, I mean the amount of graphics memory used, not performance, but yeah, probably not really worth it. Fun side note, I actually had it as a transformation, but explicitly thought splitting it into different elements was a good idea, see the discussion in #234 🙂 I think I will just change it to a Mat4 instead of the other parameters, which is not a super nice breaking change, but well, nothing to do about it. If we at some point want to do the actual optimisation, it's still possible, just split the matrix into the different parts and only transfer those that contribute to a transformation.

That is a bit weird that it doesn't implement Mat4 to Mat3 conversion. Anyway, if I do the change described above, you should be able to avoid it 💪

I will look at the example shortly 👍

[iwanders]

By optimisation, I mean the amount of graphics memory used, not performance

Ah, different background / domains, for me optimisations and performance always refer to CPU and memory lookups / allocations.

If we at some point want to do the actual optimisation, it's still possible, just split the matrix into the different parts and only transfer those that contribute to a transformation.

I think this will be tricky; The scaling component in the 4x4 matrix means the top left 3x3 is no longer a pure rotation matrix, and splitting it into individual parts may not be trivial (if at all possible). I've never tried it though, I just scale the meshes themselves and then all 4x4 transforms are normalized. If you want to easily allow for this future optimisation, I'd make the translation optional and add Option<Mat4>. But it's your call on how you want to change this, if you change it at all, the current API also works :)

[asny]

Yeah you are right, it's not trivial to split it. However, the common use-case is to only apply translation to each instance (for example if you have a point cloud), and in that case, I can check to see if all the Mat3 are identity and then only send a translation per instance. I've done that in #296 , do you want to take a look and give some feedback if you have any?