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

Particle system - Part4 #615

Merged
merged 14 commits into from
Feb 18, 2021
Merged

Conversation

caguero
Copy link
Contributor

@caguero caguero commented Feb 5, 2021

This pull request adds the ability to change the properties of an existing particle emitter at runtime.

See examples/worlds/particle_emitter.sdf for testing.

particles_mod

Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
@chapulina chapulina added enhancement New feature or request rendering Involves Ignition Rendering labels Feb 5, 2021
@adlarkin adlarkin self-requested a review February 9, 2021 01:03
include/ignition/gazebo/components/ParticleEmitterCmd.hh Outdated Show resolved Hide resolved
src/rendering/RenderUtil.cc Outdated Show resolved Hide resolved
include/ignition/gazebo/rendering/SceneManager.hh Outdated Show resolved Hide resolved
src/rendering/RenderUtil.cc Outdated Show resolved Hide resolved
src/systems/particle_emitter/ParticleEmitter.cc Outdated Show resolved Hide resolved
src/systems/particle_emitter/ParticleEmitter.cc Outdated Show resolved Hide resolved
Comment on lines 260 to 270
// Create an entity.
auto entity = _ecm.CreateEntity();
if (entity == kNullEntity)
{
ignerr << "Failed to create a particle emitter entity command" << std::endl;
return;
}

_ecm.CreateComponent(
entity,
components::ParticleEmitterCmd({this->dataPtr->userCmd}));
Copy link
Contributor

@adlarkin adlarkin Feb 9, 2021

Choose a reason for hiding this comment

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

As I mentioned in my other comment, I think it would be better to make the particle emitter command a component of the original emitter entity defined in Configure instead of creating a separate entity just for the command. I'm not sure what difficulties this change would introduce, though. I'm assuming that there may be difficulty deleting the component once the update has been applied and re-defining the component if new commands are given.

Copy link
Contributor

@iche033 iche033 Feb 17, 2021

Choose a reason for hiding this comment

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

done. a07c2ba

I ran into an issue with removing the ParticleEmitterCmd component from the entity, which still needs to be resolved

Copy link
Contributor

@adlarkin adlarkin Feb 17, 2021

Choose a reason for hiding this comment

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

I ran into an issue with removing the ParticleEmitterCmd component from the entity, which still needs to be resolved

Interesting, so the component is never removed? Maybe the new msgCmd component wasn't added to the entity yet, and we are trying to remove the component too soon? For example, would it make more sense to remove components after updating the emitters with the msgCmd in RenderUtil::Update()?:
https://github.com/ignitionrobotics/ign-gazebo/blob/a07c2ba39162191bacfe2fb74f1e1236124c0dac/src/rendering/RenderUtil.cc#L573-L577

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's an issue with updating components on the gui side. Adding a OneTimeChange in the particle system when setting component data seem to made sure the new values are propagated to the gui. Updating the emitter properties should work now after f8a971d

test/integration/components.cc Show resolved Hide resolved
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@iche033 iche033 mentioned this pull request Feb 17, 2021
include/ignition/gazebo/components/ParticleEmitterCmd.hh Outdated Show resolved Hide resolved
src/gui/plugins/scene3d/Scene3D.cc Show resolved Hide resolved
src/rendering/RenderUtil.cc Outdated Show resolved Hide resolved
src/systems/particle_emitter/ParticleEmitter.cc Outdated Show resolved Hide resolved
test/integration/particle_emitter.cc Outdated Show resolved Hide resolved
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
@nkoenig nkoenig mentioned this pull request Feb 17, 2021
7 tasks
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@adlarkin
Copy link
Contributor

adlarkin commented Feb 18, 2021

Everything seems good. @iche033 Quick question before merging: is the one time change fix for updating components a temporary solution, or is it a permanent fix?

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@iche033
Copy link
Contributor

iche033 commented Feb 18, 2021

Quick question before merging: is the one time change fix for updating components a temporary solution, or is it a permanent fix?

I'm not exactly sure if it's the right fix so I added a note in 4142eea

@adlarkin adlarkin merged commit 3484b7f into particle_system_part3 Feb 18, 2021
@adlarkin adlarkin deleted the particle_system_part4 branch February 18, 2021 01:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request rendering Involves Ignition Rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants