ClassiCube / MCGalaxy

A Minecraft Classic / ClassiCube server software
GNU General Public License v3.0
172 stars 80 forks source link

Main command has (perhaps) unwanted side effect to unload old main level #627

Closed johannespartin closed 3 years ago

johannespartin commented 3 years ago

Currently the main command is unloading the old main level when setting the new one. This is not described in help and cannot be toggled.

Proposal would be to add an optional argument "unload" to the main command and adjusting the help texts.

Goodlyay commented 3 years ago

Can you elaborate on how the current behavior is an issue? It's standard for /any/ map to unload once people are no longer in it.

Goodlyay commented 3 years ago

Looking at your commit, I do notice it is odd that the "auto unload" property is automatically set to true on the old main when you set a new one. I agree that it should not do that, but the other edits seem unnecessary to me.

johannespartin commented 3 years ago

@Goodlyay From my perspective main is the pointer to the 'default' level. While main is always loaded, there could be many maps loaded. If you play on multiple levels at the same time, changing the main pointer will have the effect that a loaded map (the old main) is unloaded, while I may still want to have players on this level.

Goodlyay commented 3 years ago

If players are in the old main when you set a new one, those players are not kicked out of the old main. It only unloads once everyone leaves.

johannespartin commented 3 years ago

@Goodlyay But if they would leave and would want to rejoin again, you would need to load it again.

Goodlyay commented 3 years ago

If you want players to be able to join or rejoin maps freely, you should keep "Load on /goto" checked. image Otherwise, yes, it will be your responsibility to load levels before they can be accessible. If you want to keep a level from automatically unloading once everyone leaves, you can use "/map unload" to toggle the auto-unload setting to false.

I will restate though that I agree "/main [level]" shouldn't be tampering with the unload settings of the original map, I just don't see the purpose of the extra toggle in the /main command itself.

Goodlyay commented 3 years ago

Your edit of the help for /main is also misleading, as using the "unload" option won't necessarily unload main, it will just make it unload if everyone leaves it.

johannespartin commented 3 years ago

@Goodlyay Thank you for outlining that! Highly appreciate the discussion.

That was just an proposal to still have the option. But I agree that it is somehow misleading. Your idea would be to just remove the unload part in the Set main function?

oldMain.Config.AutoUnload = true; oldMain.AutoUnload();

Goodlyay commented 3 years ago

Yes, that is pretty much my thoughts. Except I'd still want to keep oldMain.AutoUnload(); since the old main should unload if no one is in it when you switch mains. (Maps normally only check to auto unload when people leave them, so this special case needs a reminder)

Goodlyay commented 3 years ago

Via 70b5565a14d1cbf0574238e48bdc90f8f003b46c /main [level] no longer tampers with the auto unload setting on the old map. Does this address the issue?

johannespartin commented 3 years ago

Works for me. Thank you. I'll close my PR