neoforged / NeoForge

Neo Modding API for Minecraft, based on Forge
https://projects.neoforged.net/neoforged/neoforge
Other
1.04k stars 134 forks source link

Add Modded Experimental Feature Flag #1106

Open Soaryn opened 3 weeks ago

Soaryn commented 3 weeks ago

Summary

Add a FeatureFlags.MODDED_EXPERIMENTAL for mods to have not production ready content without having to reinvent the wheel on disabling things in builds.

Premise

Feature flags in Minecraft have a lot of positive side effects for indicating a block, item, or feature is not quite ready for normal use.

Things that Feature flags provide out of the box that I need:

The idea is that all mods would share a singular Experimental Feature flag that is intended for production level testing, but only for users who ACTUALLY intend to test incomplete features. These features are not meant to be toggled on and off, granularly controlled, or used in normal gameplay. This is purely a perpetual "Not Yet Implemented" feature flag for 1.21 and on. Pre-1.21 there was a 1.21 Experiments flag, that is now gone with 1.21 being released. Constantly picking a random one seems not quite correct, given the two vanilla feature experiments are not "incomplete" but rather undecided.

Instead of everyone who wants to have a feature locked system making their own derivation of isEnabled overrides, it would be extremely helpful to have one centralized flag controlling beta or experimental behavior. This also captures all of the aforementioned checks as well as likely several other places without a developer having to worry about it.

TelepathicGrunt commented 3 weeks ago

My stance:

Feature flags are problematic where you can’t assign them to individual mods without running out. It is not scalable for mods.

If you instead make mods all be shoved into a single flag, you lose the ability to turn on only some mod’s features and not others. A feature flag would be removing choice from mod pack makers. Not giving it. Generally, this is disliked.

Also, say you have an experimental feature in your mod. And want to test with another mod in dev environment for mod compat purposes. And that other mod also has an experimental feature that is straight broken and crashes the game. Now you cannot disable that mod's feature while trying to test your own. You are effectively stuck. This is bad.

Alternative suggestion:

With startup configs and all the other stuff like conditional load in JSON files etc, there isn’t much reason to do a feature flag when you can do a config and fully disable a feature. This allows modpack makers and players to disable broken experimental features while being able to play and test other features. This gives choice back to pack makers and players.

Bonus, you can attach comments to configs to state all the possible side effects and issue with turning on an experimental config option so people can make an informed decision.

If you wanted the feature flag solely because it has a GUI component, then this is absolutely the wrong direction to go. I would recommend using Configurate mod for a config GUI until we take on and complete this issue report on adding a native NeoForge config GUI https://github.com/neoforged/NeoForge/issues/956

Gaming32 commented 3 weeks ago

The downside with early configs is that they automatically apply to all worlds and also require a restart. If you want to test an experimental feature in one world, for example, but accidentally load into the wrong world, you could potentially corrupt worlds. My suggestion would be to add a dedicated feature flag API that would include patching the system to support more than 64 feature flags.

TelepathicGrunt commented 3 weeks ago

@Gaming32

My suggestion would be to add a dedicated feature flag API that would include patching the system to support more than 64 feature flags.

This was attempted before in past. The suggestion isn't new and 2 maintainers worked together and really tried hard to expand the feature flag limit to allow a feature flag per mod. It ultimately failed due to a bunch of implementation issues that rose from that and was scraped. So we are stuck with 64 limit unless Mojang changes how feature flags works under the hood to be nicer to us.

Edit: Got detail on what the original issue was. "Extending the limit is trivial, the issue is that feature flags are set in stone very early during vanilla's loading process before any mod has started loading, so no mod can actually register anything"

Gaming32 commented 3 weeks ago

Mods could register feature flags in a file, similar to how enum extension is now being done.

Minecraftschurli commented 3 weeks ago

Mods could register feature flags in a file, similar to how enum extension is now being done.

That is not what is being requested here, you can open your own feature request for it but IMO it has even less chance of being done than this request

Soaryn commented 2 weeks ago

If you instead make mods all be shoved into a single flag, you lose the ability to turn on only some mod’s features and not others. A feature flag would be removing choice from mod pack makers. Not giving it. Generally, this is disliked.

This is not meant to give pack makers a choice, it is literally a "Don't use this in production", but allow it to be toggled simply on dev side without having to jump through hurdles to make sure we match all of vanilla's existing infrastructure. It is only meant to test features that are not ready. If a player or pack maker forces it on, that is breaking the use case and which case typically should be ignored.

Also, say you have an experimental feature in your mod. And want to test with another mod in dev environment for mod compat purposes. And that other mod also has an experimental feature that is straight broken and crashes the game. Now you cannot disable that mod's feature while trying to test your own. You are effectively stuck. This is bad.

If you load up a world in your dev environment, you should not be loading a world you care about or have a habit of backing up a world. Dev environments are not play servers. I do see your point on if you wanted to test just your exp features with another mod without theirs, but honestly, they shouldn't be built in a crashed load state, but more is "unexpected behavior beyond this point".

Alternative suggestion:

With startup configs and all the other stuff like conditional load in JSON files etc, there isn’t much reason to do a feature flag when you can do a config and fully disable a feature. This allows modpack makers and players to disable broken experimental features while being able to play and test other features. This gives choice back to pack makers and players.

Startup configs is a different problem addressed really. This is not meant for pack makers. Though this system would be nifty, it is not the focus of this issue. This is not meant to be granular or controlled by packs. It is meant for a player to say "Hey! I may want to try this potentially broken set of things from mods. Oh it crashed, well I guaranteed didn't nuke my production world"

Bonus, you can attach comments to configs to state all the possible side effects and issue with turning on an experimental config option so people can make an informed decision.

Informed decisions are nice... when people actually read. I can put big bold letters of red on my stream for the precise pack I am playing, and people will still ask what am I playing. Moments later after I asked, they then realize OH there is text there that tells me. Yes, this is common. I do think comments are good, but this is again more for your toggleable feature scalpel concept, vs the requested bazooka.

If you wanted the feature flag solely because it has a GUI component, then this is absolutely the wrong direction to go. I would recommend using Configurate mod for a config GUI until we take on and complete this issue report on adding a native NeoForge config GUI #956

No, gui is nice for this, but the main focus is to be using the same infrastructure vanilla provides with a non-changing experiment. Consider when vanilla gets rid of bundles and trades. Do we twiddle our thumbs? This is intended to ensure there is always an experimental flag available. As right now the "response" I typically get is "use bundles" which gives no indication there are potentially crashing or not ready implementations yet, but rather a single item.

Minecraftschurli commented 2 weeks ago

As right now the "response" I typically get is "use bundles" which gives no indication there are potentially crashing or not ready implementations yet, but rather a single item.

this is also bad because a lot of people enable the bundle because they want to use the bundle

FiniteReality commented 2 weeks ago

I'd like an experimental feature flag, so that I can control content that I haven't completed, or even decided if I want yet. It's intentionally not something that modpack makers are supposed to control: the whole point is that it's a flag to say "Hey, I've got code here that's completely untested and likely broken! It'll probably make your save very sad, and may crash your game on repeat!" - modpack authors aren't supposed to be making those kinds of decisions for players.

Yes, a config could be used, but again, that's controllable by modpack makers, and it also means we as developers have to go into every place where FeatureFlags are checked, and most likely, mixin our own code because it's somewhere completely separate from the code we're actually writing.

Ultimately, the whole point behind this feature is to make modders' lives easier, and reduce the amount of code needed to clearly indicate "HEY, DON'T USE THIS UNLESS YOU KNOW WHAT YOU'RE DOING" - isn't that the whole point behind NeoForge?

Shadows-of-Fire commented 2 weeks ago

It is meant for a player to say "Hey! I may want to try this potentially broken set of things from mods. Oh it crashed, well I guaranteed didn't nuke my production world"

This underscores the problem with a catch-all experimental feature flag. How do users know what the flag is actually going to turn on?

With the proposed solution, you wouldn't even be able to know what mods are providing flagged content, let alone what content is enabled in said mod(s) as a result of enabling the flag.

Soaryn commented 2 weeks ago

The user would see it is "Not yet implemented" and should be encouraged to not turn it on. Again, if you want a granular system, I am all for it, but then that infrastructure needs to be built, where as the flags currently exist, and could be the base line solution for the catch alls. As an extra note, this is only on NEW worlds. So a user would be making some pretty trivial choices if things crash.

Minecraftschurli commented 2 weeks ago

I have not seen an argument made that would outright disqualify the concept so I'd say it's worthy of a PR and we'll see how the implementation would look