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

Attempt #6 at a Fabric Data Fixer API #3649

Draft
wants to merge 38 commits into
base: 1.20.5
Choose a base branch
from

Conversation

Treetrain1
Copy link

@Treetrain1 Treetrain1 commented Mar 14, 2024

This is a copy of #3644 with changes to use a single schema tree across mods to remove isolation issues across datafixers.
After testing, I believe this is the key to the schema registration isolation issue because the base schema is now the last schema of the last registered data fixer, except for the first registration of course because the parent will just be the vanilla data fixer.

Possible concerns:

  • Mods could break the data fixers of other mods by creating a custom schema that cancels the registration of block entities or entities. This would theoretically only affect the mods that have registered a data fixer before the problematic mod.
  • Mods could just not register their block entities and entities, limiting the effectiveness of data fixers.

Todo:

  • Write testmod implementation for the "isolation" issue
  • Fix existing testmod's biome fixes for 1.19.3+

Treetrain1 and others added 25 commits March 11, 2024 19:00
This allows mods to register more than one datafixer by specifying a key
Ex: When mod "fabric-api" registers a data fixer with key "yes", it saves the data version as "fabric-api_yes"

This commit also uses a universal method to fix tags, so nothing is left out. However, passing through `WORLD_GEN_SETTINGS` does cause some issues with some mods in very specific cases.
Provides events to register block entities and entities
Also update the lookup testmod to register the inspectable pig entity type to the schema
The events never have a reasonable time to register to, so entrypoints are probably the better option.

The entrypoint is now called in `FabricDataFixesInternalsImpl` only. They do not register in the no-op internals.
Registration now uses `Identifier`s instead of strings.

Will be used to add choices to the vanilla datafixer.
Also remove schema registration from other testmods
This is not currently tested and may be reverted
@Treetrain1
Copy link
Author

Treetrain1 commented Mar 27, 2024

The only major concern I have left is that since the schemas are in a single parent line, any mod could cancel the registration of block entities and entities within the schemas by not calling super.registerBlockEntities or super.registerEntities in a schema.
Correct me if I am wrong, but wouldn’t this break all of the parent data fixers?

@Treetrain1
Copy link
Author

Treetrain1 commented Mar 27, 2024

Also, I’m wondering what the best way to get modders to register their block entities and entities to the schema would be. Perhaps an integration into the Fabric docs website wherever it talks about block entities and entities and an announcement somewhere.
In any case, if this is merged, I think modders should get notified in some way about this.

@Treetrain1
Copy link
Author

Question.
The vanilla block entity and entity builders have a String id parameter that is used to find an instance of the id in the schemas.
Fabric basically ignores this with its builders and builds anyway, so I’m wondering if this should be implemented alongside this PR.

This is already included in this PR (though likely controversial since it logs an error), but I’m wondering what any of your thoughts are on this.

@Treetrain1 Treetrain1 marked this pull request as ready for review March 28, 2024 01:40
Treetrain1 and others added 6 commits March 27, 2024 20:53
This will hopefully reduce concerns about requiring modders to use the Data Fixer API. If a modder does not want to see warnings in the log, they can simply remove the given identifier.
value = "INVOKE",
target = "Ljava/util/List;forEach(Ljava/util/function/Consumer;)V",
ordinal = 1,
shift = At.Shift.BEFORE

Choose a reason for hiding this comment

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

The shift is presumably unnecessary.

ordinal = 1,
shift = At.Shift.BEFORE
),
locals = LocalCapture.CAPTURE_FAILHARD

Choose a reason for hiding this comment

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

Never use LocalCapture, at all. MixinExtras' @Local is preferable but this should be a @ModifyReceiver anyway and it does not need locals.

@apple502j apple502j added the priority:low Low priority PRs that don't immediately need to be resolved label Apr 4, 2024
@Treetrain1 Treetrain1 marked this pull request as draft April 15, 2024 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:low Low priority PRs that don't immediately need to be resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants