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

API for registering custom world preset editors #9436

Merged

Conversation

Commoble
Copy link
Contributor

@Commoble Commoble commented Apr 1, 2023

Minecraft provides the ability for datapacks to declare "world presets", a list of default dimensions over which individual dimension jsons (also from datapacks) are then merged on top of. World presets can be chosen when creating a new save. This system replaced the older "world type" system.

For singleplayer worlds, minecraft also allows specific world presets to be further edited by the user via guis assigned to world preset IDs; currently, this "registry" is hardcoded to an immutable map of guis for vanilla's flat, buffet, and single biome world presets. This map is an immutable map in a static final field in an interface, making adding additional screen factories to this map infeasible as it currently stands.

To allow mods to add PresetEditors (screen factories) for their own world presets, we create our own map, fire an event during client init to register additional PresetEditors, and redirect vanilla's queries to forge's map.

Mods can subscribe to the RegisterPresetEditorsEvent (mod bus, client only) to register their own PresetEditors. A test mod is included with a very basic example screen that allows the user to select a swamp world or a desert world.

image

@autoforge autoforge bot added Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. 1.19 Feature This request implements a new feature. New Event This request implements a new event. labels Apr 1, 2023
@autoforge autoforge bot requested a review from a team April 1, 2023 19:55
Copy link
Member

@pupnewfster pupnewfster left a comment

Choose a reason for hiding this comment

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

I haven't done any testing yet though does seem like it will end up being useful, but left a couple comments/questions about various parts of the code.

*/
public void register(ResourceKey<WorldPreset> key, PresetEditor editor)
{
this.editors.put(key, editor);
Copy link
Member

Choose a reason for hiding this comment

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

I don't know enough about usage people would have, but is this something that would make sense to validate against overriding? The main reason I ask is that because vanilla's entries get added before the event is fired which means that modders can override the preset editor's for vanilla presets which just feels wrong when the name itself specifies they are meant as a specific "preset"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"PresetEditor" means editor of a WorldPreset, not editor-that-has-already-been-set.

I like the idea of allowing mods to override the vanilla editors here but we can open that to further discussion

Copy link
Member

Choose a reason for hiding this comment

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

I will defer then to both you and other people who do world gen mods and the like what use cases there are and if it makes more sense for this to be overridable or not, but will give it at least a few days to give other devs a chance to weigh in on this

Copy link
Member

@pupnewfster pupnewfster left a comment

Choose a reason for hiding this comment

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

Tested this and it seems to work, but one small change that should be made for the test mod is adding a lang value for the preset generator.custom_preset_editor_test.custom_preset_editor_test just so that it is clearer to people trying to figure out how it works from the test mod that a lang entry should also be set.

@pupnewfster pupnewfster removed the Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. label Apr 9, 2023
@autoforge autoforge bot added the Assigned This request has been assigned to a core developer. Will not go stale. label Apr 9, 2023
@autoforge autoforge bot added the Needs Rebase This PR requires a rebase to implement upstream changes (usually to fix merge conflicts). label May 4, 2023
@autoforge autoforge bot removed the Assigned This request has been assigned to a core developer. Will not go stale. label May 4, 2023
@autoforge
Copy link

autoforge bot commented May 4, 2023

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

@autoforge autoforge bot removed the Needs Rebase This PR requires a rebase to implement upstream changes (usually to fix merge conflicts). label May 5, 2023
@autoforge autoforge bot requested a review from pupnewfster May 5, 2023 19:57
@autoforge autoforge bot added the Needs Rebase This PR requires a rebase to implement upstream changes (usually to fix merge conflicts). label May 12, 2023
@autoforge
Copy link

autoforge bot commented May 12, 2023

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

@autoforge autoforge bot removed the Needs Rebase This PR requires a rebase to implement upstream changes (usually to fix merge conflicts). label May 12, 2023
@Shadows-of-Fire Shadows-of-Fire added the Assigned This request has been assigned to a core developer. Will not go stale. label May 14, 2023

@OnlyIn(Dist.CLIENT)
public interface PresetEditor {
+ @Deprecated /** @deprecated see {@link net.minecraftforge.client.PresetEditorManager} */
Copy link
Member

Choose a reason for hiding this comment

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

Just a note: Patches do not have to be minimized, as long as the # of hunk lines is not changed. Merging these on the same line is not necessary. Compare this to, for example, what we do in BuiltinRegistries.java.patch. Having a Forge: prefix is preferred for the case where mapping providers document the field alongside Forge, and then it is good to know which is which or where something came from (even if it is obvious in the case).

}

/**
* {@return Retrieves the PresetEditor for the given WorldPreset key, or null if no such PresetEditor exists.}
Copy link
Member

Choose a reason for hiding this comment

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

Another note: This makes the javadoc output look like Returns Retrieves the PresetEditor for the given WorldPreset key, or null if no such PresetEditor exists..

The trailing period is added by javadoc for you. Additionally, the verb of Returns is already present, so another verb should be avoided.

@SizableShrimp SizableShrimp merged commit 8abe9b6 into MinecraftForge:1.19.x May 14, 2023
justliliandev added a commit to justliliandev/MinecraftForge that referenced this pull request May 16, 2023
justliliandev added a commit to justliliandev/MinecraftForge that referenced this pull request Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.19 Assigned This request has been assigned to a core developer. Will not go stale. Feature This request implements a new feature. New Event This request implements a new event.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants