[1.21.4] Tear out ExistingFileHelper
#1799
Merged
+259
−689
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Remove usages of `ExistingFileHelper` is as many neoforge/test data providers as possible
ApexModder
added
cleanup
Last commit published: 3ee5bc79a62d18f0cb60ed631cf47b541bd1d847. 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 #1799' // https://github.com/neoforged/NeoForge/pull/1799
url 'https://prmaven.neoforged.net/NeoForge/pr1799'
content {
includeModule('net.neoforged', 'neoforge')
includeModule('net.neoforged', 'testframework')
}
}
}MDK installationIn order to setup a MDK using the latest PR version, run the following commands in a terminal. mkdir NeoForge-pr1799
cd NeoForge-pr1799
curl -L https://prmaven.neoforged.net/NeoForge/pr1799/net/neoforged/neoforge/21.4.53-beta-pr-1799-pr-kill-existing-file-helper/mdk-pr1799.zip -o mdk.zip
jar xf mdk.zip
rm mdk.zip || del mdk.zipTo test a production environment, you can download the installer from here. |
ApexModder
changed the title
ExistingFileHelper and mark nullable everywhereExistingFileHelper
|
PR has been updated to fully tear out |
|
@ApexModder, this PR introduces breaking changes. Compatibility checks
|
marchermans
previously approved these changes
Dec 24, 2024
|
I'm just going to go on record saying I extensively use the existing file helper to run datagen based on existing files. Be it taking in 1 or more existing images and generating a new image based on them, or taking in existing structure NBT and modifying it for output, or even in some cases taking a JSON file and manipulating it for a generated file. Removing this API would break every single one of those, you need to provide a replacement unless you are just planning to piss off modders again by removing APIs we are actively using. I am not the only one making use of existing files for datagen, it's useful for far more than checking if a file is present. We don't need anything fancy, just needs to be able to fetch a resource from either our own resources, vanilla resources, or a mod dependency resources |
Minecraftschurli
previously requested changes
Dec 24, 2024
There was a problem hiding this comment.
Query the community for if this is still being used and requires a replacement
(like @KnightMiner is saying he makes extensive use of it)
Add back `assetIndex`/`assetsDir` to `clientData` run config args Add back `DataGeneratorTest` sound/particle tests making use of new resource manager
This comment was marked as outdated.
This comment was marked as outdated.
XFactHD
requested changes
Dec 25, 2024
src/main/java/net/neoforged/neoforge/common/data/AdvancementProvider.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/common/data/SoundDefinitionsProvider.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/data/event/GatherDataEvent.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/common/data/DataResourceManager.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/common/data/DataResourceManager.java
Outdated
Show resolved
Hide resolved
|
Here is an example of accessing resources for usage in datagen: https://github.com/AztechMC/Modern-Industrialization/blob/5d9c1b69f3d6d683554f2003f6c688b89909e676/src/client/java/aztech/modern_industrialization/datagen/texture/TexturesProvider.java#L73. I think it's really simple, and doesn't require any specific NeoForge extension. @KnightMiner |
Construct data resource managers directly as `MultiPackResourceManager`
…lper` Use vanillas `AdvancementProvider` instead now
XFactHD
approved these changes
Dec 27, 2024
ApexModder
dismissed
Minecraftschurli’s stale review
Dismissed due to ResourceManager providing what community used ExistingFileHelper for
|
🚀 This PR has been released as NeoForge version |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow up PR from #1725 which aims to deprecate
ExistingFileHelper(EFH) further with no planned replacement.This is done by marking EFH as
@Nullablein as many places as possible and providing overloaded constructors to allow constructing providers without needing to pass a EFH instance.All NeoForge and test providers have been updated to use the new overloaded constructors, this is to validate that everything still works when no EFH instance is provided.