Refactor Feature to have a common, supporting FeatureBase

[magicus]

The Features had a lot of duplicated code just to get the framework up. I've created a more useful abstract base class, FeatureBase, which makes the actual features just having to declare if they need key holders or subscribe to events.

Now the simplest features has really obvious and transparent functionality, with all the boilerplate hidden.

I've also applied some additional cleanup/refactoring, like adding {} after if statements, and better enforcing of the "if TEST return" pattern we've been using.

[Incompleteusern]

A few comments

[kristofbolyai]

A few days ago, I was working on automated feature registry with reflection code. I didn't get far though, I only tested a few libraries for scanning for classes (org.reflection, google.guava and ClassGraph).

Do you think that FeatureRegistry should be changed to have automated feature registration? If so, is that in the scope of this PR?

Second thing, in the LR PR (#50) we had disagreements about how we could easily enable and disable features. Maybe we could add framework for that as well. I remember Incompleteusern mentioned that we need an easy way to be able to turn on and off features (for configs), and I agree. Currently we only have the list of the features. In the LR PR, I changed that to be a Map, so we could access feature instances more easily. Do you think there is a better solution to that (as you mentioned that you did not like those changes)?

[Incompleteusern]

A few days ago, I was working on automated feature registry with reflection code. I didn't get far though, I only tested a few libraries for scanning for classes (org.reflection, google.guava and ClassGraph).

Do you think that FeatureRegistry should be changed to have automated feature registration? If so, is that in the scope of this PR?

Second thing, in the LR PR (#50) we had disagreements about how we could easily enable and disable features. Maybe we could add framework for that as well. I remember Incompleteusern mentioned that we need an easy way to be able to turn on and off features (for configs), and I agree. Currently we only have the list of the features. In the LR PR, I changed that to be a Map, so we could access feature instances more easily. Do you think there is a better solution to that (as you mentioned that you did not like those changes)?

I think we should either use the Condition system already present in Feature for enabling, but its problems exist as mentioned above

[magicus]

@kristofbolyai We can probably revisit how we registers Features. I'm not convinced we should replace the explicit registration, but maybe. In any case, let's do that separately.

Feature enabling/disabling will need to be a thing, yes. I think it feels like it's quite tightly coupled to configuration. Like when you enable a feature, you can also chose to configure it.

Did anyone look at if we could use any of the existing config frameworks (like Cloth or whatever they are called nowadays), or if we should port over the old? The biggest advantage of the old config system is the visuals, I think. It does not look Vanilla at all, but it ties in quite nicely with the Wynntils/Wynncraft look. The part how this is tied in with the actual variables carrying the configuration could certainly do with some changes, though...

[kristofbolyai]

Did anyone look at if we could use any of the existing config frameworks (like Cloth or whatever they are called nowadays), or if we should port over the old?

I am pretty sure we concluded that we should port the old one or make a new one, and that frameworks do not fit our needs. (it's a discussion in the Configs PR, which was closed)

[magicus]

The problem with the Map for features in the Lootrun PR, I think, is that it's a kind of special thing that we want to "dig up" a Feature instance from the outside. I don't really like that. Nobody should really "depend" on a Feature, so they should not care about getting access to a Feature instance. And if we really, really, need it, this should be solved within the feature. Perhaps the LootrunFeature constructor could set a public static instance field. As long as we ever only creates one instance, this works fine.

[P0keDev]

if we're going to group config values in their respective Feature classes, i think having some way to access the instance of a feature would be useful

regarding our old config stuff, i would disagree that the gui was designed well - it was pretty cramped and difficult to navigate, in my opinion

[magicus]

@P0keDev The Features are available from the registry. So if you want to loop over all features and setup configurations, that is no problem. The problem is if you want to access the instance of a specific Feature. But I think that is, essentially, misguided.

[Incompleteusern]

Features should be one instance

[P0keDev]

well accessing a specific feature would be required to access config variables inside it, no? unless you think the whole idea of keeping config values inside features is misguided

(referring to code outside a feature that might depend on its settings)

[magicus]

@P0keDev @kristofbolyai @Incompleteusern Any more comments, or is this good to be committed?

[DevScyu]

well accessing a specific feature would be required to access config variables inside it, no? unless you think the whole idea of keeping config values inside features is misguided

(referring to code outside a feature that might depend on its settings)

Should code outside a feature depend on feature settings? Isn't the whole point of them being separated into wynncraft / Minecraft and feature so that stuff like that wouldn't happen?

Could you give an example of where this would happen for example?

(if I'm completely wrong sorry I'm a little outside the loop and need to catch-up)

[Incompleteusern]

WynnGearStack and LootrunUtils are examples. Second example ie less of a concern as enabling a feature externally is basically guaranteed.