Skip to content
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.1] Allow mods to add custom FeatureFlags #1538

Open
wants to merge 10 commits into
base: 1.21.1
Choose a base branch
from

Conversation

XFactHD
Copy link
Member

@XFactHD XFactHD commented Sep 12, 2024

This PR makes it possible for mods to add custom FeatureFlags to the vanilla feature flag system. Since the FeatureFlagRegistry holding the flags is set in stone long before mods start loading, the flags are specified in a JSON file instead of being collected by an event or similar system. To allow adding more than the 64 flags allowed by vanilla (a limitation that is not unlikely to be hit with 100s of mods in a pack), the flag masks are extended by a long[] in the FeatureFlagSet and an offset field in FeatureFlag for indexing into the array.

This is an alternative solution to #1435.

TODO:

  • Test network checks and loading error handling
  • The flags pack test has no way to request confirmation that the experiments screen showed the packs or would show them
  • Find a way to make the experiments screen scrollable:
    grafik

@XFactHD XFactHD added enhancement New (or improvement to existing) feature or request data driven This request involves a data driven system 1.21.1 Targeted at Minecraft 1.21.1 labels Sep 12, 2024
@neoforged-pr-publishing
Copy link

neoforged-pr-publishing bot commented Sep 12, 2024

  • Publish PR to GitHub Packages

Last commit published: a3c8c129be590ac62c04915c30b74f49cac5c6da.

PR Publishing

The artifacts published by this PR:

Repository Declaration

In order to use the artifacts published by the PR, add the following repository to your buildscript:

repositories {
    maven {
        name 'Maven for PR #1538' // https://github.com/neoforged/NeoForge/pull/1538
        url 'https://prmaven.neoforged.net/NeoForge/pr1538'
        content {
            includeModule('net.neoforged', 'testframework')
            includeModule('net.neoforged', 'neoforge')
        }
    }
}

MDK installation

In order to setup a MDK using the latest PR version, run the following commands in a terminal.
The script works on both *nix and Windows as long as you have the JDK bin folder on the path.
The script will clone the MDK in a folder named NeoForge-pr1538.
On Powershell you will need to remove the -L flag from the curl invocation.

mkdir NeoForge-pr1538
cd NeoForge-pr1538
curl -L https://prmaven.neoforged.net/NeoForge/pr1538/net/neoforged/neoforge/21.1.76-pr-1538-modded_feature_flags/mdk-pr1538.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.

@XFactHD XFactHD linked an issue Sep 12, 2024 that may be closed by this pull request
@XFactHD
Copy link
Member Author

XFactHD commented Sep 19, 2024

Found a somewhat decent way to make the experiments screen scrollable. The scrollable variant only applies when modded flags are present, otherwise the vanilla screen is used.
grafik

@neoforged-automation
Copy link

@XFactHD, this pull request has conflicts, please resolve them for this PR to move forward.

@ApexModder
Copy link
Contributor

ApexModder commented Sep 25, 2024

Could we possibly provide in some testmod elements locked behind a custom flag, to validate that custom flags are working and disabling elements correctly.

Something simple like a item which prints a message to chat

  • right clicking items should not work if they are disabled

or even a block & block item

  • placing blocks should not work if they are disabled
  • block items should auto inherit the status from their associated block; meaning you should only need to set the flag on the block, no need to also set it on the block item

@XFactHD
Copy link
Member Author

XFactHD commented Sep 25, 2024

Adding a test item protected by one of the test flags seems sensible to me, but it should be sufficient to test that Item#isEnabled() returns false. If that test works but any of the other behaviours break, then we've got significantly larger problems.

@ApexModder
Copy link
Contributor

Yeah that would also be reasonable, I just went to test in dev real quick and realized there was no item to give, so couldn't validate if test flags were being enabled/disabling elements correctly.

Copy link
Contributor

@ApexModder ApexModder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@XFactHD could this be rebased onto 21.2. Would like to get this merged while we are in BC phase.

@XFactHD XFactHD changed the base branch from 1.21.x to 1.21.1 October 23, 2024 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.21.1 Targeted at Minecraft 1.21.1 data driven This request involves a data driven system enhancement New (or improvement to existing) feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Modded Experimental Feature Flag
2 participants