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.2] Modded feature flag #1435

Closed
wants to merge 29 commits into from

Conversation

ApexModder
Copy link
Contributor

@ApexModder ApexModder commented Aug 8, 2024

This PR introduces a custom flag system for marking elements as locked behind a set of required flags.

Although vanilla provides a FeatureFlag system, modders should not use it in its current state. This PR offers a similar system specifically for modders.

To use this system, a registry element must implement FlagElement and provide a set of required flags via the requiredFlags method. All vanilla feature elements have been patched to support this new flag system directly. For example, Items and Blocks can be assigned a set of flags during registration through their Properties objects.

To check if a given flag is enabled, you need an instance of FlagManager, which can be obtained via a MinecraftServer instance. The isEnabled method of feature elements has been patched to check if their respective flags are enabled.

All currently enabled flags are appended to crash reports, making it easy for modders to know which crashes are potentially caused by flagged elements and which are not

All flags are disabled by default. To enable a flag, a server admin must use the command /neoforge flag enable <flag>.

A new ICondition has been implemented (RequiredFlagsCondition), which allows marking data files as requiring a set of flags.
Due to when flags are loaded and how they are currently toggled, after setting a flags state a /reload will be required to reload the data files with the correct flag states.

Example mod - This mod registers a new Block and Item to the game, which are both locked behind a new modded flag flag_example_mod:experimental.

Note: New PR since original #1322 was closed, was running under port/1.21.1


TODO

  • Enable flags via data packs
    • Scrapped as deemed not needed for modded usage.
    • Flags are enabled via server admin command and data files can be conditionally loaded using RequiredFlagsCondition.

@ApexModder ApexModder added enhancement New (or improvement to existing) feature or request breaking change Breaks binary compatibility request for comments For gathering opinions on some topic or subject 1.21.1 Targeted at Minecraft 1.21.1 awaiting community agreement labels Aug 8, 2024
@neoforged-pr-publishing
Copy link

neoforged-pr-publishing bot commented Aug 8, 2024

  • Publish PR to GitHub Packages

Last commit published: 1e4128c895ae93e539a7e6c1f660043459786f35.

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 #1435' // https://github.com/neoforged/NeoForge/pull/1435
        url 'https://prmaven.neoforged.net/NeoForge/pr1435'
        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-pr1435.
On Powershell you will need to remove the -L flag from the curl invocation.

mkdir NeoForge-pr1435
cd NeoForge-pr1435
curl -L https://prmaven.neoforged.net/NeoForge/pr1435/net/neoforged/neoforge/21.1.72-pr-1435-pr-modded-flags/mdk-pr1435.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.

@neoforged-automation
Copy link

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

@sciwhiz12 sciwhiz12 added 1.21.2 Targeted at Minecraft 1.21.2 and removed 1.21.1 Targeted at Minecraft 1.21.1 labels Aug 25, 2024
Implement FeatureElement extensions
Implement saving/loading flag states to/from disk
Add coloring to flag list command
Remove ability to mark flags as enabled by default
Add basic java docs
Add parented element patches
Extend test to actually implement a GameTest

GameTest validates the following
- flag can be enabled and disabled
- all flagged elements match the expected enabled state

register flagged block & entity type
Add java doc to Flags
Update deprecated comments
Move flag manager events to NeoForgeEventHandler
Migrate SavedData over to LevelAttachment
Formatting
Fix junit tests hanging
Append enabled flags to crash reports
Reapply Item and MobEffect patches
Tear out old system
Initial implementation of overhauled system
Namespace the flags
Remove old unneeded json file
Fix data gen crashing
Fix junit tests hanging in CI
Update to use reference sets
Reapply MobEffect patches
Append modded feature flags to crash reports
Add RequiredFlagsCondition to allow marking data files as requiring a set of flags
@ApexModder ApexModder marked this pull request as ready for review September 5, 2024 14:41
@ApexModder ApexModder changed the title [1.21.1] Modded feature flag [1.21.2] Modded feature flag Sep 5, 2024
@ApexModder ApexModder requested a review from embeddedt September 5, 2024 17:08
Add back the vanilla MenuType constructor
RequiredFlagsCondition now works during initial server start up :D
Flag states are saved and loaded from WorldDataConfiguration similarly to feature flags.
Remove old level data attachment.
@ApexModder ApexModder requested a review from XFactHD September 6, 2024 11:43
Copy link
Member

@FiniteReality FiniteReality left a comment

Choose a reason for hiding this comment

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

Most of the code seems pretty good, but I think there's a few awkward pitfalls where you're doubling/tripling allocations for no reason, or caching things without measuring the performance of doing so.

* @return The unique identifier for this {@link Flag flag}.
* @apiNote This identifier does not need to be globally unique, only unique to the owning mod.
*/
public String identifier() {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of having separate identifier/namespace methods, these should return the backing ResLoc, to enable access to the rest of the ResLoc API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But would anyone ever need access to the rest of the RL Api from a flags identifier as context? I'm pretty sure the answer is no but if not I can easily update this to return the full RL

The only reason a RL is used here is for namespace prefixed identifier parsing, I could drop the RL and do all that myself in the codec/stream codecs. Just found it easier to reuse a RL for this though.

patches/net/minecraft/client/Minecraft.java.patch Outdated Show resolved Hide resolved
patches/net/minecraft/server/MinecraftServer.java.patch Outdated Show resolved Hide resolved
src/main/java/net/neoforged/neoforge/flag/FlagCommand.java Outdated Show resolved Hide resolved
src/main/java/net/neoforged/neoforge/flag/FlagManager.java Outdated Show resolved Hide resolved
Copy link
Member

@XFactHD XFactHD left a comment

Choose a reason for hiding this comment

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

We need to make sure that all codepaths reached by "flag enabled" checks (which can be called in hot paths) actually take advantage of the Flag interning by using ReferenceSets. It may even make sense to drop the equals() and hashCode() overrides entirely due to the interning.
It's also worth noting that some codepaths don't use FeatureElement#isEnabled() and instead call FeatureElement#requiredFeatures() to check it against another flag set, one example being ClientPacketListener#isFeatureEnabled(). This needs to be taken into account for the modded flag checks.

public boolean equals(Object obj) {
if (this == obj)
return true;
if (obj.getClass() == Flag.class)
Copy link
Member

Choose a reason for hiding this comment

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

Flag is final, there is no benefit to paying the overhead of an instanceof

src/main/java/net/neoforged/neoforge/flag/FlagManager.java Outdated Show resolved Hide resolved
FeatureFlagSet requiredFeatures = FeatureFlags.VANILLA_SET;
@Nullable
BlockBehaviour.OffsetFunction offsetFunction;
+ private final it.unimi.dsi.fastutil.objects.ReferenceSet<net.neoforged.neoforge.flag.Flag> requiredFlags = new it.unimi.dsi.fastutil.objects.ReferenceOpenHashSet<>();
Copy link
Member

Choose a reason for hiding this comment

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

Even if we use fastutil reference sets, my point from before still stands; the requiredFlags fields should be implemented in such a way that we are not storing a bunch of empty ReferenceOpenHashSet objects for blocks without any required flags. The memory usage will add up quickly in situations where there are a lot of single-state blocks (which is common with building block mods, for instance).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, if no flags are defined why store a empty set for every block (all of vanilla would be have said empty sets).

I could update to have a global immutable empty set stored statically in FlagManager which everything delegates to if no other flags could be found/are defined.

Copy link
Member

Choose a reason for hiding this comment

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

The simplest solution would be to make the field non-final and either initialize it to ReferenceSets.emptySet() and only replace it with a mutable set the first time a flag is added or patch the block constructor to replace the mutable set with ReferenceSets.emptySet() if it's empty. The former will likely be easier to use in other places, especially ones were no builder-like structure is involved. It's also worth noting that blocks are a particularly annoying case because they keep around the BlockBehaviour.Properties object for their entire lifetime. This means that you need to discard the mutable set on both the block and its properties if it's empty and also avoid copying the flags set when transferring it to the block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since blocks keep the properties around for their whole lifce cycle, for them specificly would it not be better to delegate to the set within the properties rather than constructing and storing a new one in the block itself?

Everything else i can update to match as xfact suggested, empty by default but once a flag is defined its updated to a mutable set

Copy link
Member

Choose a reason for hiding this comment

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

Sure, keeping it only on the properties also works. Make sure to update the properties copying to take the initial empty set into account.

blockbehaviour$properties.spawnTerrainParticles = blockbehaviour$properties1.spawnTerrainParticles;
blockbehaviour$properties.requiredFeatures = blockbehaviour$properties1.requiredFeatures;
+ blockbehaviour$properties.requiredFlags = new it.unimi.dsi.fastutil.objects.ReferenceOpenHashSet<>();
+ blockbehaviour$properties.requiredFlags.addAll(blockbehaviour$properties1.requiredFlags);
Copy link
Member

Choose a reason for hiding this comment

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

Won't this produce an empty ReferenceOpenHashSet when copying a Properties object which had no required flags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yeah, if theres no flags on the thing we are copying it would still be mutable. I can fix that

@neoforged-automation
Copy link

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

@ApexModder
Copy link
Contributor Author

Closing as this has been replaced with #1619 and #1538

@ApexModder ApexModder closed this Nov 13, 2024
@ApexModder ApexModder deleted the pr/modded-flags branch November 21, 2024 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.21.2 Targeted at Minecraft 1.21.2 awaiting community agreement breaking change Breaks binary compatibility enhancement New (or improvement to existing) feature or request needs rebase request for comments For gathering opinions on some topic or subject
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants