Fix setting particle size #241
Conversation
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Codecov Report
@@ Coverage Diff @@
## ign-rendering4 #241 +/- ##
==================================================
+ Coverage 52.59% 52.87% +0.28%
==================================================
Files 143 143
Lines 13329 13563 +234
==================================================
+ Hits 7010 7172 +162
- Misses 6319 6391 +72
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Looks good!
Update: Take a look at my comment below that follows up on my initial testing remarks here.
I would like to update the particle emitter test. Right now, the current test creates a particle emitter, checks to make sure it was created properly, modifies all particle emitter properties, and then checks the newly modified emitter to make sure that everything was update correctly. I think that we should add a test case where we create an emitter, but then modify only the type and then only the particle size. That way, we can make sure that all of the other properties of the emitter are still correct if only the type and/or particle size is modified. @iche033 I can take care of updating this test if you'd like (just let me know!).
| // area emitter | ||
| ParticleEmitterPtr areaEmitter = _scene->CreateParticleEmitter(); | ||
| areaEmitter->SetType(EM_BOX); | ||
| areaEmitter->SetEmitterSize({3.0, 3.0, 3.0}); | ||
| areaEmitter->SetLocalPose({3, 0, 0, 0, -1.5707, 0}); | ||
| areaEmitter->SetRate(10); | ||
| areaEmitter->SetParticleSize({0.01, 0.01, 0.01}); | ||
| areaEmitter->SetLifetime(1); | ||
| areaEmitter->SetVelocityRange(0.5, 1); | ||
| areaEmitter->SetMaterial(particleMaterial); | ||
| areaEmitter->SetColorRangeImage(RESOURCE_PATH + "/smokecolors.png"); | ||
| areaEmitter->SetScaleRate(1); | ||
| areaEmitter->SetEmitting(true); | ||
| root->AddChild(areaEmitter); |
There was a problem hiding this comment.
Does the tutorial need to be updated at all to make reference to this new area emitter?
|
|
||
| this->SetEmitterSize(this->emitterSize); | ||
|
|
||
| // set other properties |
There was a problem hiding this comment.
I believe we need to call SetDuration and SetEmitting here as well, since these methods set properties for this->dataPtr->emitter.
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
|
I went ahead and addressed some of my feedback in a19125e. I wanted to follow up about my previous comment regarding tests. So, it looks like the particle emitter unit test tests the data members that belong to the |
I'm trying to keep the tests generic so we can run them for different rendering engines. The other option is an integration test to test before and after changing type or particles, e.g. if you have a point particle emitter with a duration or 1 sec and then you change it to a box emitter, you should see the particles more spread out while only being enabled for 1 sec as well. |
Good point, I didn't think about that 👍
That's not a bad idea. The only thing to consider with an integration test like this is that we should try to test all of the emitter properties, not just duration (particle size, velocity, scale rate, etc.), which I think would be very difficult to test. So, we can either write an integration test that checks for some of these things, or just leave everything as-is (i.e., just keep the unit test). I'm not sure what would be better. |
|
I'm leaning towards keeping the test as is given the amount of effort needed and time to get other particle effects changes in. Let's revisit this in the future. I've ticked an issue: #249 |
|
Most of this looks good to me, but before merging, @iche033 can you take a look at my feedback/review comments above and changes I made in a19125e? In particular, I'd like to get your confirmation about this change. If all the changes look good to you, then I am okay with approving and merging this. |
the changes look good, thanks! |
Summary
Setting the size of the particles after the particle emitter is created had no effect. The solution is to destroy and recreate the particle system with the new size.
I also updated the code so that the ParticleEmitter class does not create all different types of particle emitters (e.g. point, box, ellipsoid, etc) when it's initialized. When the user changes the emitter type, we'll destroy the existing emitter and create a new one with the specified type.
Note: my original intention was to have a flag to mark the emitter as dirty when the above params changed and let
PreRender()handle the recreation of the emitter. However it was found that overridingPreRender()broke ABI so I worked around it with a manualPreRenderImplcallChecklist
codecheckpassed (Seecontributing)
test coverage)
another open pull request
to support the maintainers
Note to maintainers: Remember to use Squash-Merge