Opposition to 5c5f364a3dba553b3ddcafdce893bd38a66738cb

[PikaMug]

Respectfully opposing the change introduced by 5c5f364a3dba553b3ddcafdce893bd38a66738cb, whereas Effect no longer has a "particle" parameter.

While it's understood that many effects ignored this parameter or did not function well with it (ImageEffect), removing the ability to check or set the particle for most effects without having to know their class is detrimental to external libraries.

For example, observe the following code from QuestsEffect:

public Effect adjustEffect(Effect effect) {
        if (effect.particle.equals(Particle.NOTE)) { // effect.particle no longer exists
            effect.particleData = 0.5f;
        }
}

As you can see, I can't adjust the NOTE particle without first knowing the class type, which is problematic when particles of various kinds are being frequently displayed.

What I would prefer to see, and think would be best here, is that the "particle" parameter make a comeback, and those classes that use it merely override the variable. For example, in IconEffect.java, instead of ...

public Particle particle = Particle.VILLAGER_ANGRY;

... just do ...

public IconEffect(EffectManager effectManager) {
    super(effectManager);
    particle = Particle.VILLAGE_ANGRY;
}

This would alleviate the problem without the breaking changes. If it's preferable that the default particle not be FLAME, then we merely don't set it as such the base Effect class. I would be happy to submit a PR for this if desired :)

[NathanWolf]

The change to consolidate the particle property was done without my knowledge in the name of a huge "cleanup" commit. I think it was a mistake and I don't plan to bring it back.

I will leave this open and consider alternatives, but in general I don't think it's too much to ask that you know what you're modifying before you modify something.

The same argument could be made for any parameter- why can't I set a "radius" on every effect, and just have the ones that don't use a radius ignore it?

One alternative approach would be to use the configuration-based approach, which basically works like you describe. I think unless you turn on debug messages it will ignore missing properties silently, which sounds like what you're looking for?

The other thing I take issue with in general is EffectLibs idea of public properties as an API- that's just horrible to begin with. So if you can't or won't use the configuration-based API, we could consider instead adding a method such as setParticle() that each Effect could implement (even the ones without a "particle" parameter, such as Atom). This would be a bit of a pain, but really adding accessors for all of the properties would be best at this point, versus this horrible public property stuff that should've never existed.

[PikaMug]

Whereas the end user is specifying the effect type, I can't know which class is being modified. Checks for every effect class that uses particles (or I suppose filtering out those that don't) would be bloat and need updating if/when new effects are added. And I'm not sure I agree with the analogy - most effects use particles, whereas few use radius. For particle-less effects, I think just setting particle == null would suffice, which is what I assume would happen under the hood if you switched to accessors?

[NathanWolf]

You are asking me to put back something that I consider a bug added by another dev- I'm really not going to do that, sorry.

[NathanWolf]

Some possible ways to work around this, may or may not fit your use cases:

1. use the configuration-based effect construction, like magic does
2. use reflection to see if a class has a particle property (if you really must)

I apologize for being resistant, but you happen to have started relying on something that should never have existed ... I think it was in there for about a year, so not exactly a short time, but still what I would consider a temporary bug (or at least a design flaw) that didn't exist before and won't exist again.

From the sound of what you're doing, the configuration approach may work best, I'm not sure if you can modify an effect in the way that you're doing it with configs, but that could be added.

And I get what you're saying about radius vs particle, but it's still a fine line. I don't want the superset of all config options to exist in the base class just so you can blindly set any of them, whether or not they actually matter to a given effect implementation.

[NathanWolf]

Relevant xkcd, to hopefully add some lightheartedness to this conversation 😅

[PikaMug]

Good xkcd, but do know that I'm not trying to press anything on you. I can always maintain a separate fork.

Iirc I did look at the config-based construction, but it wasn't really conducive to how basic I wanted/needed settings to look for the end user. Here's a portion of the QuestsEffect config to give you an idea:

# QuestsEffect configuration. See https://tinyurl.com/effect-classes for class values.

effects:
  npc:
    new-quest:
      enabled: true
      class: MusicEffect
      height-offset: 0.0
      if-applicable:
        particle: note
        color-source: '#123456'
        color-target: '#abcdef'
    redo-quest:
      enabled: true

As you can see, it's up to them to set the effect class. Resorting to reflection would be icky.

[NathanWolf]

Well.. I don't want to make your life difficult just because I'm being obstinate.

I'm honestly probably just grumpy about this because the change in question broke all of the default effects and was part of this mega "optimized stuff" type commit, so I couldn't just revert it, and fixing the issue was quite tedious.

Un-fixing the issue in the way you suggest (moving default particle initialization to the constructor) will probably also be a little tedious, but otherwise is a fine solution.

I don't want you to have to maintain your own fork just because I'm being stubborn about some nebulous design principle that really doesn't matter much at the end of the day.

So that all said, let's go ahead and make that change. If you're up for doing it and PR'ing that would be excellent, if not I totally understand and I will try to get to it as soon as I can.