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

Added dynamic dimension registering #7371

Closed
wants to merge 4 commits into from

Conversation

maltesermailo
Copy link

Hello Forge team and other contributors,

I've added dynamic dimension registering because I saw it necessary, as certain mods depended on the flexibility of registering dimensions on the fly while the server runs. In this example I created a working prototype to register dimensions when needed or on server start.

The System

In 1.16 Mojang has changed a lot to the dimension registration. Essentially they have added the possibility for dimensions to be statically created using datapacks. While this opened up the possibility to make custom dimensions for users, for modders it changed a lot because now it wasn't possible anymore to dynamically create dimensions with custom chunk generators.

With my DimensionManager it will be possible again to create dimensions with pre-created json dimension types while the server is running or on server start. This makes it possible for modders to create custom dimensions specifically for one player(e.g. a pocket dimension).

The system works by hooking into the original process at the point after the WorldSettings are read in from the level.dat file/when the level.dat is created for the server or before the integrated server gets started for the client. Then it calls the DimensionRegisterEvent on the Main Event Bus for Mods to listen to. Mods then have the ability to access the DynamicDimensionManager for registering dimension definitions and loading them when created after server already started. The Event is called before the level.dat file is saved again after datapacks and dimensions have loaded. The dimension definition then gets saved in level file. When the dimension definition is registered during the event, the server will automatically create the ServerWorld instance on server startup, otherwise it is possible using the DynamicDimensionManager#loadOrCreateDimension(ResourceLocation) method.

How to create a dimension

Dimension registering is really easy with the system. Just hook into the DimensionRegisterEvent on the normal forge event bus and you can register your dimensions easily
`@SubscribeEvent
public void onDimensionRegister(DimensionRegisterEvent e) {
long seed = 0;

    //Registers the dimension if it not exists already in the registry.
    e.getDimensionManager().registerDimension(new ResourceLocation("dimension_test", "testworld"), 
    			new Dimension(() -> e.getDimensionManager().getDimensionType(new ResourceLocation("minecraft", "overworld")), 
    					new NoiseChunkGenerator(BiomeProviders.createOverworldBiomeProvider(seed, false, false), seed, () -> {
    				         return e.getDimensionManager().getDimensionSettings(new ResourceLocation("dimension_test", "testworld"));
    				      })));
}`

This for example creates a normal overworld dimension at the resource location dimension_test:test_world using the dimension type minecraft:overworld and my custom noise settings. Since the creation of a chunk generator requires to access the Biome Registry for Overworld Chunks, the BiomeProviders utility class helps to create BiomeProviders without needing to access the Biome Registry manually.

If you want to register a dimension while the server is already running, you just need to call the registerDimension method on the DynamicDimensionManager and then the loadOrCreateDimension method to create the world instance.

`DynamicDimensionManager.getDimensionManager().registerDimension(new ResourceLocation("dimension_test", "testworld"),
new Dimension(() -> DynamicDimensionManager.getDimensionManager().getDimensionType(new ResourceLocation("minecraft", "overworld")),
new NoiseChunkGenerator(BiomeProviders.createOverworldBiomeProvider(seed, false, false), seed, () -> {
return DynamicDimensionManager.getDimensionManager().getDimensionSettings(new ResourceLocation("dimension_test", "testworld"));
})));

		DynamicDimensionManager.getDimensionManager().loadOrCreateDimension(e.getServer(), new ResourceLocation("dimension_test", "testworld"));`

Thanks for reading my pull request.

@CLAassistant
Copy link

CLAassistant commented Sep 27, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@Cyborgmas Cyborgmas left a comment

Choose a reason for hiding this comment

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

Mostly was picky on code style, but also am not sure how this can be used for dynamic dims since the test mod in no way does that.

Comment on lines +329 to 330
+ net.minecraftforge.dimension.DynamicDimensionManager.getDimensionManager().onIntegratedServerStop();
+ net.minecraftforge.fml.client.ClientHooks.handleClientWorldClosing(field_71441_e);
Copy link
Contributor

Choose a reason for hiding this comment

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

There is already hook exactly here, would make more sense to instead put in inside handleClientWorldClosing.

Copy link
Author

Choose a reason for hiding this comment

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

yep, that would be a good idea. gonna do it

* Holds various builder functions for Biome Providers
*
*/
public class BiomeProviders
Copy link
Contributor

Choose a reason for hiding this comment

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

this class doesnt seem all that necessary... i doubt a lot of mods will want to use any of these...

Copy link
Author

Choose a reason for hiding this comment

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

I just included for the use-case of creating a NoiseChunkGenerator. Don't want to let them grab the minecraft BiomeRegistry(not the Forge one, the constructor actually needs Registry) manually and create an OverworldBiomeProvider.

import net.minecraftforge.common.MinecraftForge;
import net.minecraftforge.event.dimension.DimensionRegisterEvent;

public class DynamicDimensionManager
Copy link
Contributor

Choose a reason for hiding this comment

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

forge classes dont use tabs as indent, but 4 spaces.

* Loads or creates the specified dimension.
* @param dimension
*/
public void loadOrCreateDimension(MinecraftServer server, ResourceLocation loc)
Copy link
Contributor

Choose a reason for hiding this comment

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

you have some weird indentation and spacing going on in this class. Try and keep it consistent

Copy link
Author

Choose a reason for hiding this comment

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

That might have been a fuckup of GitHub Desktop. In my file on my local pc, the lines of the method have all the same indentation.

* For default biome providers see {@link BiomeProviders}
* @return the biome registry
*/
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps explain why this is deprecated?

Copy link
Author

Choose a reason for hiding this comment

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

That's so modders don't get the idea of using this getter to get direct access to the BiomeRegistry but instead use my BiomeProviders class in case they want to create a standard BiomeProvider. Although I need it for my BiomeProviders class itself. Could have changed that to protected though.

@Nullable
public World getServerWorld(MinecraftServer server, ResourceLocation loc)
{
Validate.notNull(server, "Must provide server when creating world");
Copy link
Contributor

Choose a reason for hiding this comment

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

why "when creating a world"? you say in the doc you retrieve the instance, I would assume this means the instance has already been created.

Copy link
Author

Choose a reason for hiding this comment

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

Might be me thinking multiple things at once and then mixing up. Must mean when retrieving world.

import net.minecraftforge.eventbus.api.Event;

/**
* Register your custom dimension definitions when you receive this event.
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for so much space in the docs, plus wrap that huge line on multiple ones instead.

@@ -46,7 +46,7 @@
.executes(ctx -> {
ctx.getSource().sendFeedback(new TranslationTextComponent("commands.forge.dimensions.list"), true);
final Registry<DimensionType> reg = ctx.getSource().func_241861_q().func_243612_b(Registry.field_239698_ad_);

Copy link
Contributor

Choose a reason for hiding this comment

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

you added a tab here, remove it to clean the diff

Copy link
Author

Choose a reason for hiding this comment

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

argh, i've changed something here to test the dimension registry but reverted it later, apparently eclipse replaced the spaces with tabs, i'm sorry, i already changed settings to only use spaces in future on that project.

@SuppressWarnings("unused")
@Mod(DimensionTest.MODID)
@Mod.EventBusSubscriber(bus = Bus.FORGE)
public class DimensionTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

you only create a dimension in the event you post before the server is created and when the server is starting, i fail to grasp how this is a "dynamic" dimension creation. I would imagine this should be able to create a dimension/world at any point during the game. Also, if this system is capable of creating dimensions on the fly, I would suggest stress testing this, by seeing how much time it takes to create different dimensions (i.e. a second overworld vs a second End) or when creating 2 dimensions at once. I would assume that creating 2 dims at once is pretty heavy and would require some sort of work queue, where they arent processed all at the same time and dont cause the game to freeze forever.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your comment. I used the ServerStartingEvent because it is called exactly after spawn chunks are loaded and the Done message pops up in server log(at least on Dedicated). Therefore I thought it was suitable for testing it after world loading. Of course it should work outside of the ServerStartingEvent too, but I thought this was plausible with this example. The dimension adding shouldn't be too heavy on the server. The server will just tick it in the next tick, when creating a dimension, loading chunks as necessary when the player gets ported there or when a chunk is forced. I don't think that anyone will ever create more than 2 dimensions at once. That wouldn't make any sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah okay, since it is a one off thing per world load it did not seem quite dynamic.
and in a server it could very well happen, 2 players using RF Dimensions and creating one at the same time or similar times. I would just debug it to make sure. This is some pretty heavy stuff and has a lot of potential for problems as i understand it. The more you test the more seriously this can be considered. However this is just my opinion.

Copy link
Author

Choose a reason for hiding this comment

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

From my tests, it takes about 6 seconds per dimension to create it. A reasonable amount of time for creating a load of chunks, though if you want, I can add that it forces the spawn chunks to be loaded at world creation. When both worlds are registered at the same tick they might get created both.

@50ap5ud5
Copy link
Member

50ap5ud5 commented Sep 28, 2020

Thanks for adding this! I've tested this successfully by adding a new dimension when right clicking a block.

I've noticed that the Experimental warning in the WorldOptionsScreen will always display when the DimensionGenerationSettings were made in json (i.e. a json in data/modid/worldgen/noise _settings). This occurs when you create the dimension, quit the game and reload the world save that contained your dimension. This is because the WorldOptions Screen is hardcoded to show the warning if the lifecycle result of a DataResult<DimensionGeneratorSettings> is experimental (which is true for all json based ones).

This is not really an issue with the PR, but I can see this being a user friendly issue for some. Any thoughts on this?

@Cyborgmas
Copy link
Contributor

Cool that it works then, I would just add to the test mod what soap did, to show a bit more "dynamism" in action.
For the warning there is already a PR, if you want to review it /upvote it #7275

+ return;
+ }
+
+ IChunkStatusListener ichunkstatuslistener = this.field_213220_d.create(11);
Copy link
Contributor

Choose a reason for hiding this comment

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

It would probably be best to use the same listener as all the other worlds, this could easily be done by creating an IChunkStatusListener field that is assigned in MinecraftServer#func_240800_l__

Copy link
Author

Choose a reason for hiding this comment

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

The IChunkStatusListener isn't really used except in loadInitialChunks which is why don't think it's necessary to keep a copy of it. I've already considered making it a NoopChunkStatusListener since it isn't used anyway, besides using memory.

Copy link
Contributor

@mrp-v2 mrp-v2 Sep 29, 2020

Choose a reason for hiding this comment

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

If you look in the ChunkManager class, the IChunkStatusListener is used every time the status of a chunk changes, like on load and unload. And if you look in the Minecraft class, it holds a reference to the latest IChunkStatusListener that is created from its factory, which makes it important to use the same instance as vanilla, otherwise vanilla's reference to its own IChunkStatusListener will be overwritten.

Copy link
Author

Choose a reason for hiding this comment

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

Well, if you would actually look at the LoggingChunkStatusListener, which is the instance made by the factory, you would see that this IChunkStatusListener does nothing when not enabled. And the only instance where it is enabled is the loadInitialChunks method to log the chunk generation for the overworld. Therefore, it is not necessary to keep it.

+ long j = BiomeManager.func_235200_a_(i);
+
+ DerivedWorldInfo derivedworldinfo = new DerivedWorldInfo(this.field_240768_i_, this.field_240768_i_.func_230407_G_());
+ ServerWorld serverworld1 = new ServerWorld(this, this.field_213217_au, this.field_71310_m, derivedworldinfo, registryKey, type, ichunkstatuslistener, gen, flag, j, ImmutableList.of(), false);
Copy link
Contributor

Choose a reason for hiding this comment

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

The way this is currently implemented does not allow mods to control the ISpecialSpawners that work in their worlds or the seed of their world.

Copy link
Author

Choose a reason for hiding this comment

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

And what purpose do they serve? I do not see any special purpose why modders should be able to pass the list to it. Nevertheless, they can also edit the list manually by using an accesstransformer to make the field public and then add it to the list.

Copy link
Contributor

Choose a reason for hiding this comment

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

ISpecialSpawners control the spawning of Phantoms, Patrols, Cats, Wandering Traders, etc.., as well as any that mods add. If mods want their worlds to be able to spawn those things, they need to be able to control the spawners passed to world creation.

Copy link
Member

Choose a reason for hiding this comment

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

@mrp-v2 How would you suggest the ability of accessing the ISpecialSpawners be implemented?

Copy link
Contributor

@mrp-v2 mrp-v2 Oct 5, 2020

Choose a reason for hiding this comment

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

Vanilla just creates new instances of each ISpecialSpawner, so I assume mods would do the same when they pass a list of ISpecialSpawners to the world creation method.

Copy link
Author

Choose a reason for hiding this comment

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

The seed is set btw when you create the NoiseChunkGenerator

@maltesermailo
Copy link
Author

It would be nice if Forge Contributors could review this. Maybe even LexManos.

@TheCurle
Copy link
Contributor

TheCurle commented Oct 6, 2020

Triage team exists for a reason. Lex does not even look at PRs unless we give him the all-clear.
As it stands, there are 3 different PRs all tackling this same issue, and it is such a considerably large change that we don't want to rush it.

Just wait.

@50ap5ud5
Copy link
Member

50ap5ud5 commented Oct 7, 2020

That's true, maltesermailo, it does take time for a PR to be reviewed. Perhaps in the meantime, you could review some of the suggested changes. Though, this PR is the only open PR that tackles the task of creating a dimension on the fly without using datapacks. The other PRs relate more to adding more settings for dimension types or biome modification.

@thejudge156
Copy link

thejudge156 commented Nov 1, 2020

Hi maltesermailo, I would like to see this functionality added as well. However I can't help but notice that the branch has conflicts before it can be merged I believe, mind fixing those? I would love to see many dimension mods spring up thanks to this PR.

@50ap5ud5
Copy link
Member

50ap5ud5 commented Nov 3, 2020

Hi maltesermailo, I would like to see this functionality added as well. However I can't help but notice that the branch has conflicts before it can be merged I believe, mind fixing those? I would love to see many dimension mods spring up thanks to this PR.

I believe the PR author was avoiding updating until the Forge team gives the go ahead. From earlier discussions it seemed like the Forge team wanted to address the following issues first:

  • Fix the vanilla bug that applies to dimensions being deleted: https://bugs.mojang.com/browse/MC-197860 (There is now a solution for this in #7527)
  • Map new dimension types to new entries and make sure that those new entries are synced to the client.
  • Deal with DimensionRenderInfo and find some way to default it to vanilla if it's missing.
  • Fix the vanilla bug where if a datapack that defined a dimension_type gets removed and the server reloaded, depending on where in the level.dat the dimension was that used the data type, all dimensions after that are discarded.

@DaemonUmbra DaemonUmbra added the Needs Rebase This PR requires a rebase to implement upstream changes (usually to fix merge conflicts). label Nov 29, 2020
@Eforen
Copy link

Eforen commented Dec 6, 2020

@DaemonUmbra Does the Needs Rebase mean that this PR has been approved to move forward if say for example @maltesermailo were to rebase their changes to the current head?

@Sm0keySa1m0n
Copy link
Contributor

What's the current status of this?

@50ap5ud5
Copy link
Member

50ap5ud5 commented Jan 18, 2021

What's the current status of this?

I don't think the original author plans to update it anytime soon. It should be noted that it is possible to achieve the functionality of this PR without it, except the dimension registering event.

@LexManos
Copy link
Member

LexManos commented Feb 16, 2021

I'm going to deny this.
Yes, I can understand the potential use cases.
However Mojang's data driven system is explicitly designed to not allow this.
The hope is that we can have conversations with Mojang and help them design a better system.
Hacking something on top that destroys all their internals is just asking for issues.

However, to address Modders.
With now over 10 years of experience under my belt, I can tell you this.
The MAJORITY of you do not need custom dimensions in the way that you think you do.
MOST of your systems can be done using a single dimension, ala compact machines.
If you DO need full dimensions then unfortunately, due to how vanilla is designed. This will require a server restart.
Dimensions are VERY heavy objects now. You should use them with care.

Maybe one day we'll find a simple solution that isn't a maintenance/code nightmare. But right now it's not looking good. My hope is Mojang sorts themselves out.

@LexManos LexManos closed this Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Rebase This PR requires a rebase to implement upstream changes (usually to fix merge conflicts).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet