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 emitter - part3 #117

Merged
merged 14 commits into from
Jul 30, 2020
Merged

Particle emitter - part3 #117

merged 14 commits into from
Jul 30, 2020

Conversation

caguero
Copy link
Contributor

@caguero caguero commented Jul 24, 2020

Here's the particle implementation for Ogre2.

Open questions:

  • SetMaterial(): Right now, we create an unlit internal material because this is the type that seems to work with the particle system. Then, when we call ParticleEmitter::SetMaterial() we should copy some of the properties of the material. I think Unlit and PBS don't share the same attributes. Should we copy the common ones? Maybe at least the texture?

    Reference: https://ogrecave.github.io/ogre/api/2.1/class_ogre_1_1_hlms_unlit_datablock.html

  • This PR doesn't implement the SetType() and SetEmitterSize() yet because the default particle emitters are contained inside the ParticleFX Ogre2 plugin. I don't think our FindIgnRendering for Ignition CMake is ready to use this plugin. Did anyone has experience using ParticleFX?

How to test it? Compile the particles_demo example and run it. You should see something similar to this:

particles

Signed-off-by: Carlos Aguero <caguero@openrobotics.org>
Signed-off-by: Carlos Aguero <caguero@openrobotics.org>
Signed-off-by: Carlos Aguero <caguero@openrobotics.org>
@github-actions github-actions bot added the 🔮 dome Ignition Dome label Jul 24, 2020
@osrf-triage osrf-triage added this to Inbox in Core development Jul 24, 2020
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@iche033
Copy link
Contributor

iche033 commented Jul 25, 2020

I made some changes in 2eb5cc2. Materials / textures should work now.

particles

@caguero caguero moved this from Inbox to In review in Core development Jul 27, 2020
@chapulina chapulina requested a review from iche033 July 27, 2020 18:05
@iche033
Copy link
Contributor

iche033 commented Jul 27, 2020

looked a little bit into loading ParticleFX plugin. The ~/.ignition/rendering/ogre2.log shows that the ParticleFX plugin is loaded. Did you have a linker error? We're not linking against it since it's a plugin. Could you push the changes you have for setting type and emitter size to a different branch?

Signed-off-by: Carlos Aguero <caguero@openrobotics.org>
Signed-off-by: Carlos Aguero <caguero@openrobotics.org>
@caguero
Copy link
Contributor Author

caguero commented Jul 27, 2020

looked a little bit into loading ParticleFX plugin. The ~/.ignition/rendering/ogre2.log shows that the ParticleFX plugin is loaded. Did you have a linker error? We're not linking against it since it's a plugin. Could you push the changes you have for setting type and emitter size to a different branch?

I created a minimum example in the particles_part3_affectors branch. It instantiates one of the default affectors defined in /usr/include/OGRE-2.1/Plugins/ParticleFX/OgreColourImageAffector.h. The code seems to compile but when running the example I get an undefined symbol error:

Error while loading the library [/usr/lib/x86_64-linux-gnu/libignition-rendering4-ogre2.so]: /usr/lib/x86_64-linux-gnu/libignition-rendering4-ogre2.so: undefined symbol: _ZTIN4Ogre19ColourImageAffectorE

Here's extra info:

nm -D /usr/lib/x86_64-linux-gnu/OGRE-2.1/OGRE/Plugin_ParticleFX.so | grep ColourImageAffector | grep _ZTIN4Ogre19ColourImageAffectorE
0000000000236dd8 V _ZTIN4Ogre19ColourImageAffectorE

Maybe I'm using it wrong...

@caguero
Copy link
Contributor Author

caguero commented Jul 27, 2020

looked a little bit into loading ParticleFX plugin. The ~/.ignition/rendering/ogre2.log shows that the ParticleFX plugin is loaded. Did you have a linker error? We're not linking against it since it's a plugin. Could you push the changes you have for setting type and emitter size to a different branch?

I created a minimum example in the particles_part3_affectors branch. It instantiates one of the default affectors defined in /usr/include/OGRE-2.1/Plugins/ParticleFX/OgreColourImageAffector.h. The code seems to compile but when running the example I get an undefined symbol error:

Error while loading the library [/usr/lib/x86_64-linux-gnu/libignition-rendering4-ogre2.so]: /usr/lib/x86_64-linux-gnu/libignition-rendering4-ogre2.so: undefined symbol: _ZTIN4Ogre19ColourImageAffectorE

Here's extra info:

nm -D /usr/lib/x86_64-linux-gnu/OGRE-2.1/OGRE/Plugin_ParticleFX.so | grep ColourImageAffector | grep _ZTIN4Ogre19ColourImageAffectorE
0000000000236dd8 V _ZTIN4Ogre19ColourImageAffectorE

Maybe I'm using it wrong...

It seems that the affectors are supposed to be used in a different way. See the next block:

Ogre::ParticleAffector *plane = this->dataPtr->ps->addAffector("DeflectorPlane");
  plane->setParameter("plane_point", "0 -50 0");
  plane->setParameter("plane_normal", "0 1 0");
  plane->setParameter("bounce", "1");

(Using the base class with setParameter)

@caguero
Copy link
Contributor Author

caguero commented Jul 27, 2020

looked a little bit into loading ParticleFX plugin. The ~/.ignition/rendering/ogre2.log shows that the ParticleFX plugin is loaded. Did you have a linker error? We're not linking against it since it's a plugin. Could you push the changes you have for setting type and emitter size to a different branch?

I created a minimum example in the particles_part3_affectors branch. It instantiates one of the default affectors defined in /usr/include/OGRE-2.1/Plugins/ParticleFX/OgreColourImageAffector.h. The code seems to compile but when running the example I get an undefined symbol error:

Error while loading the library [/usr/lib/x86_64-linux-gnu/libignition-rendering4-ogre2.so]: /usr/lib/x86_64-linux-gnu/libignition-rendering4-ogre2.so: undefined symbol: _ZTIN4Ogre19ColourImageAffectorE

Here's extra info:

nm -D /usr/lib/x86_64-linux-gnu/OGRE-2.1/OGRE/Plugin_ParticleFX.so | grep ColourImageAffector | grep _ZTIN4Ogre19ColourImageAffectorE
0000000000236dd8 V _ZTIN4Ogre19ColourImageAffectorE

Maybe I'm using it wrong...

It seems that the affectors are supposed to be used in a different way. See the next block:

Ogre::ParticleAffector *plane = this->dataPtr->ps->addAffector("DeflectorPlane");
  plane->setParameter("plane_point", "0 -50 0");
  plane->setParameter("plane_normal", "0 1 0");
  plane->setParameter("bounce", "1");

(Using the base class with setParameter)

I think I figured it out, I'll update the PR with new changes.

caguero and others added 3 commits July 28, 2020 01:38
Signed-off-by: Carlos Aguero <caguero@openrobotics.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Carlos Aguero <caguero@openrobotics.org>
@caguero
Copy link
Contributor Author

caguero commented Jul 28, 2020

@iche033, all API calls should be implemented in Ogre2 now. Could you take another look please?

smoke

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

looking good! Just a comment about setting type. Others are minor

/// the image is used.
/// \param[in] _image The color image name.
/// \sa ColorImage
public: virtual void SetColorImage(const std::string &_image) = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about calling this SetColorRangeImage. It's doing something very similar to SetColorRange but from an image. Another reason is to avoid confusion with setting the actual particle texture that they supplied when calling SetMaterial

I also noticed that this overrides SetColorRange and vice versa depending on which function is called last. Maybe we can add that to the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes sense. It would even make more sense to overload the functions but C++ doesn't allow us to overload two functions just based on the return value :/ 571a08f.

public: Ogre::ParticleEmitter *emitter = nullptr;

// \brief Ogre colour image affector.
public: Ogre::ParticleAffector *colourImageAffector = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

should we keep spelling consistent with others in ign? i.e. colour -> color. No strong opinion here since the public API uses color which is consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 571a08f. We have to be careful with the strings passed to Ogre because they need to be "colour".

{
case EmitterType::EM_POINT:
{
this->dataPtr->ps->removeAllEmitters();
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 with the remove call here, calling SetType will remove existing emitter settings, e.g. size, duration, rate, etc?

Copy link
Contributor Author

@caguero caguero Jul 29, 2020

Choose a reason for hiding this comment

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

Oh that was a bug, good catch. Fixed in 571a08f.

In order to make it work, we now keep an array of emitters, one of each supported type. When an attribute is set, we set it in each emitter but only one is visible at a time. When the function SetType() is called we switch to the appropriate emitter.

Signed-off-by: Carlos Aguero <caguero@openrobotics.org>
@caguero
Copy link
Contributor Author

caguero commented Jul 29, 2020

I think this is ready for prime time, I'm removing the draft part. Maybe I can create a tutorial in another PR.

@caguero caguero marked this pull request as ready for review July 29, 2020 17:16
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@iche033
Copy link
Contributor

iche033 commented Jul 30, 2020

made a couple of updates in fa3c8e4

  • added Destroy function to clean up the particle emitter
  • also added the color image texture path to ogre resource manager. Needed when the texture is stored in a directory different from the Ogre2Material texture.

@iche033
Copy link
Contributor

iche033 commented Jul 30, 2020

There are a couple of expectations failing when running the test with ogre2, e.g.

RENDER_ENGINE_VALUES=ogre2 ./build/ignition-rendering4/bin/UNIT_ParticleEmitter_TEST

I think it is failing because the values are invalid and so they are never set. Could you take a look?

Signed-off-by: Carlos Aguero <caguero@openrobotics.org>
@caguero
Copy link
Contributor Author

caguero commented Jul 30, 2020

made a couple of updates in fa3c8e4

  • added Destroy function to clean up the particle emitter
  • also added the color image texture path to ogre resource manager. Needed when the texture is stored in a directory different from the Ogre2Material texture.

Thanks!

@caguero
Copy link
Contributor Author

caguero commented Jul 30, 2020

There are a couple of expectations failing when running the test with ogre2, e.g.

RENDER_ENGINE_VALUES=ogre2 ./build/ignition-rendering4/bin/UNIT_ParticleEmitter_TEST

I think it is failing because the values are invalid and so they are never set. Could you take a look?

Fixed in 9ac9812.

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

iche033 commented Jul 30, 2020

there is a minor issue with DCO, otherwise looks good!

@caguero caguero merged commit 8bb934d into master Jul 30, 2020
Core development automation moved this from In review to Done Jul 30, 2020
@caguero caguero deleted the particles_part3 branch July 30, 2020 22:51
@j-rivero j-rivero removed this from Done in Core development May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants