-
-
Notifications
You must be signed in to change notification settings - Fork 198
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
[1.21] Add Experimental FeatureFlag #1167
Conversation
Last commit published: d2b5348857ba3b5c9d0f134f8f2c35be833cba29. PR PublishingThe artifacts published by this PR:
Repository DeclarationIn order to use the artifacts published by the PR, add the following repository to your buildscript: repositories {
maven {
name 'Maven for PR #1167' // https://github.com/neoforged/NeoForge/pull/1167
url 'https://prmaven.neoforged.net/NeoForge/pr1167'
content {
includeModule('net.neoforged', 'testframework')
includeModule('net.neoforged', 'neoforge')
}
}
} MDK installationIn order to setup a MDK using the latest PR version, run the following commands in a terminal. mkdir NeoForge-pr1167
cd NeoForge-pr1167
curl -L https://prmaven.neoforged.net/NeoForge/pr1167/net/neoforged/neoforge/21.0.48-beta-pr-1167-pr-experiment-flag/mdk-pr1167.zip -o mdk.zip
jar xf mdk.zip
rm mdk.zip || del mdk.zip To test a production environment, you can download the installer from here. |
I'll admit I'm mildly confused what this feature flag is supposed to be for -- and it definitely needs a better description in-code so modders know when to use it. Is it just a generic catch-all for "experiment" stuff within mods? How should a user know what to expect if they turn it on? What's an "experimental mod item" and how does it differ from a normal old experimental mod feature which is off by default but enable-able via a config? I'm not saying I'm entirely against this idea, I just think it's not at all clear -- to modders or to users -- what the flag is actually for as described in code and in-game descriptions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs much better documentation and reasoning for being added.
The PR description is too short.
There is no tests provided.
@@ -194,6 +194,8 @@ | |||
"neoforge.chatType.system": "%1$s", | |||
|
|||
"pack.neoforge.description": "NeoForge data/resource pack", | |||
"pack.neoforge.experimental.name": "NeoForge mod experiments data pack", | |||
"pack.neoforge.experimental.description": "Enables experimental mod items", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest "mod features" instead of "mod items". Better be vague here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps: Enables experimental mod features that may be incomplete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like neither of those suggested names, nor the current name, tell a user what will actually be enabled, which... is a bit of a problem. A generic flag for "enable God-know-what experimental features from every single mod you have installed" isn't very useful or intuitive to users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my big worry and pain point. I still feels experimental features should be a flag or config per-feature for easy enabling and disabling along with better understanding of what the user is getting into. 1 broken feature in one mod can easily make a pack unplayable with no way to disable it without disabling the other experimental features you DO want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, this is not to be turned on by Packs, but on world creation. So this is specifically someone turning it on for their world, if they update a mod and it crashes, that is no different than them updating a mod with no feature flag enabled and it crashing.
This is intended for "DO NOT turn this on unless you know precisely what you are doing." Granular control is something else entirely that has no current infrastructure. I a feature flag per mod could exist, cool: but that is very outside of scope nor is the FeatureFlag currently built to be able to support that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok... but who is actually going to use it then?
If this is to just test features, use a second branch in your repo. That's what branches are for. Build a jar with that test branch and give it to your testers to test with. Now you don't have to fiddle with pack finder events and moving json files around when you go to release the feature. Just merge the feature branch to your main branch and release.
If this is to have random users try random stuff from mods, not dedicated testers, then that should have granular control imo. This smells like a solution in search of a problem that doesn't exist.
Instead of having to recreate the entire infrastructure for a testing suite, a feature flag that is NOT bundles
or Villager trading tweaks
is ideal. Before it made sense to use the 1.21 feature flag, but all of these that exist are dependent on vanilla's life cycle for said feature. None of them are slated to be persistent, which is desired from this.
Even if it was, the current response is to use "bundles" for the most part, which I'd argue is a worse "catch-all" for a user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intended for "DO NOT turn this on unless you know precisely what you are doing."
My point here is that a user cannot know "precisely what they are doing" with this flag, because it enables a bunch of experimental features from countless mods in any given installation. The lack of granularity means that users can't make an informed decision about whether they ought to be turning on the flag or no -- it's not just "test this one mod's experimental feature", it's "test any experimental features of any mods that happen to be installed, all at once". I would say this whole PR makes no sense unless there's a way for modders to list to users what things from their mod the flag will enable, at the point where the user enables the flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree. While I can certainly see the need for better expression towards the user, "Not intended for gameplay" should be clear enough for someone to NOT turn on in a world they want to play, and if they want to explore a potentially broken set of features (of all the mods they have installed) they may do so turning that on. Expansion on that is possible, but you are limited in how you can say this given the screen real estate. It definitely needs to be expressed to mod pack makers to not enable.
The current solutions today:
- Tag the feature as either in
bundles
orvillager_trades
, both much less clear to a user that something incomplete or likely to crash is now being turned on. - Do a separate branch on a repository. That adds a burden on both the developer's time (non-trivial) and the player to understand "beta" is not meant to be played. See EnderIO slated as beta and not intended to be used, where most packs and users just put it in blindly thinking it is ready.
- Just don't do feature flags. This is arguably worse and a terrible argument.
Previous to 1.21 there was a 1.21 Experimental options
flag but as indicated above, the life cycle of the flag is tied to arbitrary features of said flag and are not persistent. This indicated the features were not yet ready. Vanilla literally already did what we are asking here, we are just wanting a persistent flag.
Solutions I am in favor of:
- A persistent catch all Feature Flag for a Test World Creation as this pr is addressing. Yes, any mod installed using this flag would be turned on, but at the time of turning on the flag, the user should expect it to have a high chance of breaking. If they turn on a flag, and added/updated a mod that broke, that is fundamentally no different here than what it is without the flag. This should be indicating that no gameplay should be done with this flag on, and will turn on experimental features of EVERY mod installed. Forgecraft would be an example of when the flag would be turned on.
- A granular control of feature elements. The problem here, is that this one keeps getting tossed around as if it is trivial to build, and no, it is not just "config" as there are a lot of components missing from what feature flags enable/disable. There is no current built in infrastructure for this, so if you are going to toss this one around, you need to build something substantial or show a PR that solves this. Both systems can work hand in hand and not mutually exclusive.
- Feature flags per mod: ideal scenario, but as far as I understand with what is synced for flags, it is not possible at the moment.
Of these, so far, no solutions exist that are as simple as adding a "Hey Don't Turn this On Unless Testing and not playing". This alleviates the technical burden of reinventing the ENTIRE feature flag system that vanilla has for a list of elements a mod may not finish but wants to make a build. Yes, users will turn this on. Yes users will report bugs on it; however, users already do this, with much less that are not bugs.
Alternatively, If you are up for it, we could add a few more persistent flags that are alpha
beta
experimental
to express more intent to the user and dev.
alpha
: Broken, will crash nearly 100%beta
: Not intended to crash, but is not outside of realm of possibilityexperimental
: Going into prod soon.
The problem with doing this, is that when one moves from one to the other, it may be turned off based on what the player has turned on (unless there is a dependency chain of feature flags I don't know about).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue isn't about when people shouldn't turn it on, it's about when people should turn it on. As currently written, this mod only gives users the option to turn on every experimental feature from every single currently installed mod. Which is useless for both users and modders -- for users, it means they're getting a bunch of potentially-buggy features all at once instead of the one specific one they're interested in testing out. For modders, it means that any bugs people find with the experimental features -- and we want bug reports for those so we can fix it before fully shipping the feature -- will be polluted by potential bugs from any number of other mods' experimental features. "Test every single experimental feature from every installed mod" is not a use case that users or modders generally want. In fact, many modders would be likely to reject outright a bug report from such a scenario, even if the issue is one of mod compat, unless the user could reproduce it with the other mods' experimental stuff turned off.
Per-mod or per-mod-feature feature flags are not, to my understanding, possible with how vanilla handles feature flags. Correct me if I'm wrong, as this would be the optimal solution. With that in mind, it seems like the proper solution is to make it easy for mods to configure experimental features that can be turned on independently of one another, that does not use feature flags. At present, it seems like this should be fully possible through the use of a config. The same "this is experimental and will break stuff" warning as can be put in a feature flag, can also be put in a mod config. If there are "components missing from what feature flags enable/disable" in what configs do -- you have not made them clear. Creative mode tab contents, loaded data, etc. -- all can be controlled from configs! Data in particular is trivial to control thanks to being able to use resource conditions in a pack.mcmeta for an overlay.
At the very least -- if folks do decide to go the direction of this feature flag approach, which seems to me like the worst possible approach for this -- then I would say there needs to be a way for users to see -- when enabling the experimental mod features flag -- what mods are using that flag and what they're doing with it. In other words, there has to be a description somewhere there that mods can add info to the contents of somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Translations have been updated but the values of these unfortunately can not provide all the information everyone is requesting.
@@ -194,6 +194,8 @@ | |||
"neoforge.chatType.system": "%1$s", | |||
|
|||
"pack.neoforge.description": "NeoForge data/resource pack", | |||
"pack.neoforge.experimental.name": "NeoForge mod experiments data pack", | |||
"pack.neoforge.experimental.description": "Enables experimental mod items", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like neither of those suggested names, nor the current name, tell a user what will actually be enabled, which... is a bit of a problem. A generic flag for "enable God-know-what experimental features from every single mod you have installed" isn't very useful or intuitive to users.
public static final Codec<FeatureFlagSet> CODEC; | ||
public static final FeatureFlagSet VANILLA_SET; | ||
public static final FeatureFlagSet DEFAULT_FLAGS; | ||
+ public static final FeatureFlag MOD_EXPERIMENTAL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs javadoc on what the flag is for, and how modders should actually use it -- notably, they have to register their own pack using it in AddPackFindersEvent
, which isn't necessarily obvious!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Java docs have been added stating what this flag is, how and when to use it, and stating that it is recommended that modders document their experimental features somewhere.
d979ddb
to
d2b5348
Compare
PR has been updated to include javadocs and testmods, it also now depends on #1187 to allow generating the testmod & neoforge builtin packs better |
Thanks for adding the docs. My opinion here, for whatever that's worth, is that:
|
The issue with configs is they can be set via a modpack, which is not ideal for something which should have some user input/confirmation. Users should be changing the state of feature flags not third parties. you also cant simply override FeatureFlags also have abunch of short circuiting if statements and checks throughout the game, making things like recipes, tickers etc not register/be ignored for disabled elements, which would be a pain for modders to have to implement via mixins and such for their configs
its possible to build a list of what elements require a specific flag, but i dont know how useful that would be to a end user, other than that afaik theres not much else could be provided to user the experiments screen doesnt have enough context to provide any meaningful information, nor can it be given any more information than it already has afaik but if this is preferred i could maybe look into having a tooltip shown or log flagged entries to console |
Logging entries is, imo, a bare minimum if the flag is meant as a non-mod-specific catch all. Otherwise any crash report modders get with it on is useless cause we don't know if other mods are doing experimental stuff. Still a bit confused about why configs can't be used -- to address some of your points, if a mod pack is enabling something that the config says it probably shouldn't be, that's their issue. There's probably a mod out there to automatically enable feature flags too, for folks wanting bundles in their modpack or whatever. Any sort of datapack stuff can totally be controlled by a config without issue -- just use an overlay in the mod data with a condition controlled by the config. Besides that, is the issue one of syncing what stuff should be considered enabled? Given that doing the same sorta thing that feature flags do there, given a server config, is a common use case outside of experimental features, it may be worth looking for a more general solution there. Wanting to enable/disable an item or group of items based on a server config setting isn't an uncommon use case. |
So, there are a few reasons, and the TLDR reason is that it is SCATTERED through MC's handling requiring a mod dev to basically reimplement all of feature flags to get sorta the same functionality:
While I would adore a What I believe is the right approach. Have a persistent flag in Neo that is disconnected from vanilla's flags as a start. Then eventually a config or Modded Feature Flag (separate mirrored system) like approach for being able to use a config or similar as simple to use as a feature flags, on top of this PR. The two systems are not mutually exclusive and would solve a problem more immediately with this. I've tried to use configs multiple times to solve this problem, it is non-trivial. Especially when compared to adding or removing I do understand the concerns you have, but the alternatives are either use |
It should be possible to abuse the |
Go ahead. If you can match the result of a feature flag 1:1 them I'm for it, but after trying multiple times and trying to explain that it is not that simple, I'll let you experiment 😂 |
What specifically about this PR is actually going to stop a pack maker from force enabling the flag with a tiny mod shipped with the pack and thus, impose the “experimental” features on users? Once they learn there’s hidden content behind a toggle, they will get a modder to make and release a mod to turn on the flag. Now they have all the features on and no way to pick and choose what they specifically want. I don’t see this going the way you all want it to go. The whole premise of this PR hinges on that a. Players and packmakers will actually listen or read the warning and not use it. You’re putting so much effort into telling players to not use it that, just don’t add it in first place b. That modders will actually use it for truly game breaking experimental and not Easter Egg/stable pre-release of features. Because when they do so, players and pack makers will try to get those stuff. c. And that actual modders and play testers want ALL possibly broken experimental stuff enabled in a modpack. This makes Mod A testers testing mod compatibility of the flagged stuff from Mod A next to impossible if a bunch of other mods in the pack has broken features that break the world. I don’t see actual benefits. Just footguns everywhere |
We should probably do a quick feasibility study on how hard it would be to write our own, more dynamic feature flag system (that does not need to load before registries), and then patch it in everywhere that vanilla's is currently called. If that is not very many patches, there is no reason not to just do that, and have proper mod-specific feature flags, rather than dealing with the compromise of a global flag. |
I really don't think this is useful. |
Implements a simple feature flag that mod developers can use to mark their elements as experimental.
This flag can be enabled via the ‘Experiments’ screen during level creation, similar to Mojang’s built-in flags.
Resolves issue #1106