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

Generate map IDs based on the next available ID rather than the last ID that exists. #640

Closed
mibby opened this issue Mar 21, 2017 · 7 comments
Labels
plugin Issue cause relates to a plugin. resolution: won't fix Issue will not be resolved or feature not added. type: question Issue represents a question and not a problem/request.

Comments

@mibby
Copy link

mibby commented Mar 21, 2017

@Zbob750 @aikar Would it be possible at all to tweak vanilla spigot / paper map .dat creation to generate maps based on the next available map ID rather than the last ID that exists?

I use this plugin to manage and convert images to a map board.
https://www.spigotmc.org/resources/bannerboard.20435/

It reserves a range of map IDs and pre-generates the blank files.

idrange:
  startid: 25000
  amount: 5000

Similarly, this plugin I use also generates and reserves map IDs ahead of time for the actual feature functionality.
https://www.spigotmc.org/resources/superscratch-next-generation-gambling-30-off.12250/

map-id: 30963

And this plugin, while it doesn't pre-generate and reserve map IDs ahead of time, it creates maps using the vanilla method based on the last ID that currently exists.
https://www.spigotmc.org/resources/artmap-make-custom-textures-and-paintings-with-no-resource-pack-%E2%9C%8D.26667/

My map ID range from 0 to 5,246 are from vanilla generated and created maps. Then there is a significant gap. Then from map IDs 25,000 to 30,000, those are reserved by the BannerBoard plugin for server-use. All ArtMap canvas maps and newly created vanilla maps have been starting from ID 30,000 up to now 31,425. Minecraft has a hardcoded map limit of 32,768, so problems will ensue quite shortly for me.

It would be wonderful if maps generated would actually check for the next map ID available and start from there rather than from based on the last ID that exists. So a new map could start from 5,247 and begin to fill the available / empty gap I have in-between the two sets rather than continue to edge its way closer to the hardcoded limit from the one end.

@ghost
Copy link

ghost commented Mar 21, 2017

should just go the full way and treat maps like books

@Black-Hole
Copy link
Contributor

Black-Hole commented Mar 21, 2017

Plugins should not reserve a hardcoded id range but instead use the next ids as needed. So you should state that the BannerBoard is messing with the ids and server owners should blame that plugin if they need more than 2768 maps for vanilla and plugin maps.

And I think Minecraft should use UUIDs to distinguish maps. Using it like books won't work because of the map copy feature. Copied map are linked and updated together.

@aikar
Copy link
Member

aikar commented Mar 22, 2017

not really fond of complicating this logic to make up for misbehaving plugins.

If we cater to them for things like this, then they have no incentive to stop doing bad things.

@mibby
Copy link
Author

mibby commented Mar 25, 2017

That's understandable. However there are a handful of plugins (that I am aware of) that do reserve a specific map ID for functionality to work. Logically, it makes sense to me that map IDs use what is available, not continue off the last ID that exists. I'm going to have over 20,000 IDs that are not being used all while my server breaks as I hit the hardcoded limit.

Couldn't there at least be an option to toggle to flip the logic from last existing to next available at the very least?

@mibby
Copy link
Author

mibby commented Mar 25, 2017

The next available ID is incremented based on the last existing ID number, not what is available from 0 up. Just to provide another example with no plugins installed whatsoever;

  • You have 5 map files in the world/data folder.
  • Those map files have the ids of map_0.dat, map_1.dat, map_2.dat, map_3.dat, and map_30220.dat.
  • When you create or spawn in a new blank map and then open it to render the area, it creates a map file with an ID of map_30221.dat.
  • It will never fill in the IDs in-between the two gaps as it always increments based on the last existing ID. Thus you would eventually hit the minecraft hardcoded limit of 32k while not even having 32k map files.

So if somehow the map IDs got out of order for whatever reason (plugins reserving / pre-generating later IDs), purging old map files no longer used through deleting earlier files, or even copying over a specific map.dat file from another world / internet resource, you would be out of luck?

Strictly speaking in vanilla sense (no plugins), it would make far more sense to create using the next available ID, not based on the last ID existing.

@electronicboy
Copy link
Member

electronicboy commented Mar 25, 2017

The value is actually cached in the idcounts.dat file, a "solution" for this would likely be to replace that lookup with something that actually scans the folder tree, which would actually harm the performance of the server when maps are created, especially for people who are already struggling with HDDs, especially if you ever hit a area where there are already several maps with the same ID, only alternative would be to complicate the lookup of the ID even more to implement the caching elsewhere, which once again just ends up hurting performance somewhere down the line...

I'm too busy to look deeply into when and where that value is updated, but you might be able to change the number using an NBT editor (at your own risk, and pray that it doesn't update it when something does look up an existing map (However, in the limited time I've had it doesn't look to do so, but at the same point, I've not looked into what happens if something tries to look up a map higher than that ID, so this is 100% at your own risk))

The issue is that modifying such a system in a way that not only complicates the existing logic, potentially (and most likely) hurts performance, and is hopefully something that only a handful of people will need, and will most likely generally only end up creating edge cases where it doesn't work as people desire is somewhat hard to justify, especially when the issue is caused by plugins abusing the system and not faulty logic in the server.

an alternative solution would be to modify the behavior of the caching system, which would drastically complicate the logic, as well as most likely require some form of way to perform a manual rescan, that would attempt to cache the value as well as verify that it was actually bumped sanely, which once again, would potentially complicate the logic and most likely introduce several dozen edge cases...

@mibby
Copy link
Author

mibby commented Mar 25, 2017

So changing the idcounts.dat map ID back down to a number that doesn't exist yet would resolve the problem temporarily by creating new maps with a lower ID. But wouldn't it then overwrite the later map IDs that exist or load the existing data for what is suppose to be a new map when it reaches an ID already taken? The question also exists, how did the idcounts.dat number jump from 4k to 30k+ without iterating / generating files for all the map numbers in-between?

Would a simple logic check for the idcounts.dat be a significant hit to performance?
i.e. if map id already exists in folder for that id, iterate to next available number.

That way the number could be set back down to 0 and it could check for next available ID that isn't already existing?

@zachbr zachbr added plugin Issue cause relates to a plugin. type: question Issue represents a question and not a problem/request. labels May 8, 2017
@zachbr zachbr added the resolution: won't fix Issue will not be resolved or feature not added. label Oct 12, 2017
@zachbr zachbr closed this as completed Oct 12, 2017
electronicboy added a commit to electronicboy/Paper that referenced this issue Mar 7, 2020
Upstream has released updates that appears to apply and compile correctly.
This update has not been tested by PaperMC and as with ANY update, please do your own testing

CraftBukkit Changes:
7f61a25 PaperMC#640: Fix chunk load/unload callbacks for chunk load cancellations
Machine-Maker added a commit to Machine-Maker/Paper that referenced this issue Jul 17, 2021
Upstream has released updates that appear to apply and compile correctly.
This update has not been tested by PaperMC and as with ANY update, please do your own testing

Bukkit Changes:
5662c2b3 SPIGOT-6642: Throw better message if plugin.yml is empty
aa8e0351 PaperMC#636: Add FurnaceStartSmeltEvent
52073b30 SPIGOT-6646: Issue with map palette ranges
5f772da9 PaperMC#640: Add new Causes for LightningStrikeEvent

CraftBukkit Changes:
4e18704 PaperMC#874: Add FurnaceStartSmeltEvent
ed2b91c SPIGOT-6649: Call BlockFadeEvent when Nylium fades to Netherrack
b7e3ce0 PaperMC#890: Include yaw in player's spawn location
aeb711d PaperMC#889: Fix CraftChest close() sound being replaced with open sound.
ca0fe5b SPIGOT-5561: Warning in logs when changing a Mob Spawner to Air on chunk load
2f038f2 PaperMC#886: Add new Causes for LightningStrikeEvent

Spigot Changes:
38e6c03d Rebuild patches
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin Issue cause relates to a plugin. resolution: won't fix Issue will not be resolved or feature not added. type: question Issue represents a question and not a problem/request.
Projects
None yet
Development

No branches or pull requests

5 participants