Fading ParticleEmitter's start and end color

[yaRnMcDonuts]

I added some code to allow fading a Particle Emitter's start and end color independently. Previously there was no way to easily fade a particle emitter in/out or to fade it to a different color - you could only set the start and end color. However, in order to smoothly fade out a particle emitter, the start color also needs to be able to be faded. So I added this code to smoothly fade the start and end color to a different color over a set duration in my own version of the ParticleEmitter in my project, and I thought that it might be useful to contribute to the core ParticleEmitter class.

No existing code was removed, and I just added methods for initiating the color fading process, as well as some code to the update loop to handle the color fading process.

[riccardobl]

Mhh I see, so this is an emitter-wide fade. For example you can have particles fade from fire to smoke and with this code you can also have the whole effect fade from solid to transparent.
Could you change the methods to use the set/get pattern to comform to the rest of the class?
Also updateColorFading should probably be protected or private

[yaRnMcDonuts]

Yes I updated it with your suggested changes

[riccardobl]

There are still a couple of things missing for this to be complete:

• It should implement cloning logic for the color fields in cloneFields()
• It should implement serialization logic for the new fields in write() and read()

[riccardobl]

The clone here creates a new object and it should be avoided.
Also i am not sure if this is a good approach, this code replaces the end color that is chosen by the developer and this is an unexpected behaviour, this fading should probably be applied directly in the updateParticle method to every particle's color.

[yaRnMcDonuts]

I updated it with all of your proposed changes - aside from the last thing you mentioned about not replacing the value of the particle emitter's initial start and end color. I think I might be confused as to what you are saying to do instead.

I intentionally made it so the colors you are fading to will get set as the new start/end color, so that if you call getStartColor() or getEndColor() in the middle of the color fade process, it will return the current color that is being visually output. But do you mean that I should instead keep track of a particles currentStartColor and currentEndColor without ever changing the value of startColor or endColor?

Or I could change the name of the setFadeToStartColor() & setFadeToEndColor() methods to instead be setStartColorOverDuration() & setEndColorOverDuation(), to better reflect how the method is another way of setting the startColor and endColor. Whichever you think is better.

[riccardobl]

Would it be possible to use the two start/end colors as initial colors for the fading and add just set the final colors with your methods?

I intentionally made it so the colors you are fading to will get set as the new start/end color, so that if you call getStartColor() or getEndColor() in the middle of the color fade process, it will return the current color that is being visually output. But do you mean that I should instead keep track of a particles currentStartColor and currentEndColor without ever changing the value of startColor or endColor?

Yes those shouldn't change, but you can use another field to store the current interpolated code and add a getCurrent method if you need to return the value.

[yaRnMcDonuts]

Would it be possible to use the two start/end colors as initial colors for the fading and add just set the final colors with your methods?

I think it could work like this - however, the only problem would be if the user calls the fade method more than once on the same particle emitter - the second and any following times would flash back to the startColor and endColor at the beginning of the fade process if the fade does not start from the current visible colors since the startColor and endColor will never change internally from the fading process and will no longer set the startColor and endColor equal to the final fade colors when the fade is compelte.

But it should be able to work with just adding a currentStartColor and currentEndColor as well as getter methods for both of these, and then the existing startColor and endColor can remain unchanged as the ParticleEmitters' initial/default values.

[yaRnMcDonuts]

Would it also be a good idea to make the existing setStartColor() and setEndColor() methods also set the currentStartColor and currentEndColor? Since the current colors will be the visible ones, and this would allow setStartColor() and setEndColor() to still have an immediate effect on the color, even if it has previously been faded. Otherwise, setStartColor() and setEndColor() would have no visible effect on a particle emitter once it has been faded. The setStartColor() and setEndColor() methods could also set the fadeDuration to 0 to stop the fade process and immediately go to the set colors.

[riccardobl]

You could increment the interpolation value and just interpolate from startColor to startColorToFadeTo every frame

eg

float startFadeI=FastMath.clamp(this.startEndFadeTime/startColorFadeDuration,0,1);
float endFadeI=FastMath.clamp(this.startEndFadeTime/endColorFadeDuration,0,1);
this.startEndFadeTime+=tpf;

ColorRGBA start=startColor.interpolate(startColorToFadeTo,startFadeI);
ColorRGBA end=endColor.interpolate(endColorToFadeTo,endFadeI);

ColorRGBA finalParticleColor=start.interpolate(end,particleI);

(obv with *Local methods and supporting variables (if needed) or it would create a lot of temporary objects)

[pspeed42]

I think one of the reasons you guys are struggling to find a good way to do this is because "fading all of the particles" is not really a "particle life cycle" problem. Maybe it's best done externally... and not particularly hard to do so, I guess.

For example, imagine if you modified the shader to support an overall alpha modifier then you could do this through a material parameter. But it's also not hard to have a separate control that changes the start/end colors over time.

[yaRnMcDonuts]

The main reason I opted to change the ParticleEmitter internally is because there were some protected methods that I needed access to, and also because it seemed to be the most convenient for the user to have the functionality there. I've found that nearly every particle emitter in my game ends up needing this code for color fading, since its uncommon to have a particle emitter that exists forever and doesn't need smoothly faded out at some point. Plus the control would only last for about 2-3 seconds before being discarded in many cases where the fade is brief, so my initial thought was that a control would be overkill

[yaRnMcDonuts]

(obv with *Local methods and supporting variables (if needed) or it would create a lot of temporary objects)

I think my last few changes managed to do this and get rid of the extra ColorRGBA object creation within the update loop that was previously there. I'll have to check my code again when I'm more awake, but I did test the ParticleEmitter and color fading functionality in my app since my last change, and it is at least all still working correctly after all the refactoring haha.

[pspeed42]

But the "I use it all the time" argument just means you should have a standard control.

I still say that the reason this code is so ugly and creates so many "this changes that while this other thing is using that" sort of dependencies is because you are using a mechanism that is meant to manage particle life cycle to manage emitter life cycle.

Not sure why you'd need protected access to set new start and end colors over time.

[yaRnMcDonuts]

Not sure why you'd need protected access to set new start and end colors over time.

I initially did not for this pull request, I think my first commit shows how it initially looked, but @riccardobl suggested it would be cleaner to not directly modify startColor and endColor.

My thought was that this color fading functionality is along the same lines as the functionality of the emitAllParticles() method which is also only called on occasion in an effect-like fashion. I could move all of the color fading code to a control instead, but then it would lose the extra functionality riccardo has suggested.

The other idea i had was to move the code to a new ColorInfluencer or EffectsInfluencer class that could be stored in the ParticleEmitter similar to how the ParticleInfluencer works, so then it would not clutter up the ParticleEmitter class nearly as much.
I had previously thought that a control was not optimal for short term things, but if that is not an issue then either should work.

[yaRnMcDonuts]

I copy/pasted the code into a control that takes in a ParticleEmitter, I wasn't sure where to locate it so I placed it in a controls package where ParticleEmitter is located.

[riccardobl]

So, how is this going to be implemented?
For me either way is valid. I can see this being part of the emitter, if implemented properly, or just a controller.

[yaRnMcDonuts]

Then I'll go ahead and put all the code in the control and revert the ParticleEmitter code back to master so it isn't changed.

I also named the new control ParticleEmitterEffectsControl, so if anyone ever has anymore effect-like code to be added for ParticleEmitter, it can be put in ParticleEmitterEffectsControl as well.

[riccardobl]

Sorry if i review it only now.
My question at this point is if this should this be included in the core.
It can work on its own, it doesn't need to be in the core.
However the usecase seems valid and the code seems good.

I'd like to hear other opinions.

[yaRnMcDonuts]

Sounds good,

My initial reason for putting it in core was because the feature seems like a necessary component for a basic particle emitter, (arguably more necessary than some existing features that are already within the ParticleEmitter class like .emitAllParticles() ) and I felt that, if it is not added into the core, that new users will likely overlook it and might think that our basic ParticleEmitter is insufficient since it lacks a basic fade-out functionality. I personally find it very unpolished and ugly when a model or particle is detached from the scene without a smooth transition, it feels like it it ruins the entire effect and breaks the immersion.

But I am open to other opinions as well, and am happy with whatever you decide to do :)

[yaRnMcDonuts]

Is there any desire to merge this PR still? I did not realize at the time that the build only failed due to the codacy review that looks like it has since been removed, and between my misunderstanding of that and having switched the code around a few times at the suggestion of others, I kind of gave up on this PR at the time.

I still strongly believe that this is a necessary feature to make the stock particle emitter disappear in a polished/clean way, and it seems well suited to be merged into core as a standalone Control.

[stephengold]

You'd probably get more response if you asked that question at the Forum/Hub.

[capdevon]

Hi @yaRnMcDonuts ,

I have some concerns about this class. It is not compatible with the SDK editor as it lacks conventional get/set methods and the empty constructor needed for serialization. Additionally, it extends AbstractControl, which already implements the Control interface, making it unnecessary to implement it again in ParticleEmitterEffectControl. There are also redundant methods and unnecessary checks in the controlUpdate() method.

Given these issues, I don't see how it could be a candidate for inclusion in a jme module. The features seem to cater more to personal needs rather than general user requirements, and there is no demo showcasing the benefits this component should provide. With all these defects, I cannot agree with its integration, as we risk repeating past mistakes like with the SingleLayerInfluenceMask class.

What worries me most is that there used to be more attention to these details, but now it feels like I'm the only one providing an honest review of the code. I don't want to sound harsh, and I'm not upset with you—I appreciate the work you're doing. I'm just concerned that there are very few of us left to maintain this engine now.

[yaRnMcDonuts]

I appreciate the review. I agree with some of your points, however on some I disagree.

Overall I would say that (especially in its current state) the PR did not turn out how I envisioned and certainly isn't ready to merge in its current state. There was a lot of review pulling things in different directions, and I also feel that a general misunderstanding of this feature originally lead to its misplacement as a separate control. There also didn't seem to be any express interest from others, so I never ended up coming back for a final code cleanup. I moved the code back and forth from my own project to the particle emitter class and then to a control, so the code got messy and certainly could be cleaned up.

The features seem to cater more to personal needs rather than general user requirements

I have to disagree with this. Smooth color fading is a basic feature that should be easy to do by calling the simple fadeToStartColor() method that I added to the particle emitter class. Just like we have convenience methods for things like emitAllParticles() and emitNumParticles(), we should have convenience method for smooth color interpolation.

Removing a particle emitter from the scene without fading its start/end color to be transparent results in an off putting and ugly transition when a fully opaque particle is suddenly removed from the scene with no fading whatsoever. Granted this is just a polishing detail, but it is certainly still an important one.

Expecting new users to realize this shortcoming and rewrite the boiler plate code for smooth color interpolation is not a good decision in my opinion.

So I still strongly believe that smooth fading for start and end colors of a Particle Emitter is an imperative feature to a particle system and should be included one way or another.

It is not compatible with the SDK editor as it lacks conventional get/set methods and the empty constructor needed for serialization

I also partially disagree with this sentiment as a reason to not merge this PR (especially if the functionality is left in an external control)

I ageree you are right to point out the possible serialization issues. But I do not think this issue alone is a reason to throw out the feature, as it can easily be fixed before this PR is passed.

And while it is important to consider compatibility with external editors in general (whether that be the SDK or custom editors out there), the SDK is considered an optional tool and most jme users don't even use it, so (from my understanding of the situation at the time) the decision was made to seperate JME from the SDK specifically so that contributions would not be hindered by the additional work required to implement and test everything in the SDK.

The important aspect is to make sure that any new/modified classes or controls have their read/write and serialization functionality done properly so that all general scene editing tools and jme apps will not break. So your review of this is correct, but past that, external editor support is generally not much of a consideration when working on the core engine.

and there is no demo showcasing the benefits this component should provide.

I would also be willing to make a quick example to showcase the potential for this addition. I could make a quick demo showing a fire particle emitter fading from orange/red to black/gray start/end colors to simulate the extinguishing of a fire. And then fade from black/gray to a fully transparent start and end color to simulate a smooth fade out effect.

With all that being said, I also don't really have much stake in whether this PR is merged, since I've already implemented this directly to my projects. I just thought it was an important feature to have for creating polished particle emitters in a final product so that it is as shippable and polished as possible. But considering no one else has shown equal interest in this feature, I don't think its my place to make the final decision on whether to merge this.

So if you still strongly believe that this feature is unnecessary in core, and if no one else speaks up who thinks this is as useful as I do, then I am okay closing the PR.