search

SourceWriters / RealisticWorldGenerator-Api

GNU General Public License v3.0
0 stars 0 forks source link

Error 1.17.1 Version Tuinity #1

Closed LiannOM closed 2 years ago

LiannOM commented 2 years ago

https://pastebin.com/my2T2qYJ

Lauriichan commented 2 years ago

This issue is not related to RWG, it mentions RWG classes cause RWG is the generator in this case but there is an ConcurrentModification at CraftBukkit, please contact Tuinity Support instead.

LiannOM commented 2 years ago

Same problem is with paper.

MWHunter commented 2 years ago

This is caused by RWG. You guys are accessing the Bukkit API on chunk generation which isn't allowed. There are a few ways to cleverly work around this if you think about why the exception is actually being thrown.

Lauriichan commented 2 years ago

We have to access the Bukkit API.

A small explanation how the ChunkGenerator class of Bukkit works. You use the ChunkData interface to place the blocks into the chunk that is requested to be generated. The error message you sent us here is exactly that. "CraftChunkData" is the implementation of the ChunkData interface and if we want to set a block into that interface we use the ChunkData.setBlock method which is normal. In this case we set a Material and then Bukkit created the BlockData for that. There is no work around for that and it is clearly a bug of paper.

See ChunkGenerator Documentation And yes it may be that we're using a now deprecated method (in case of 1.17) but that's the method used in all other versions below 1.17 therefore we have to use it to provide compatibility.

MWHunter commented 2 years ago

This is definitely still fixable as you can set blockdata itself instead of material. Terra doesn't have issues and fixed this (although you can't copy it's code because RWG isn't a compatible license)

Lauriichan commented 2 years ago

So yeah that's maybe something we can do but in our opinion this is not really a problem which we have to fix. This is definitely a problem of paper as it doesn't appear on any spigot instance on those versions in that case they should fix it if they broke it, it's not our fault that they change something which breaks some plugins.

We don't plan to change anything there as our main software that we optimise for is Spigot and that will not change. We've other things to worry about than fixing issues that a fork of spigot made which says that every plugin works on their fork.

MWHunter commented 2 years ago

I'm personally going to replace the hashmaps with concurrent hashmaps with reflection as I also have to use BlockData async in my plugins, which is the easiest solution IMO. I disagree that this paper issue shouldn't be fixed when the majority of servers are running a paper based server. Asynchronous chunk generation is vanilla behavior... spigot just removed it in 1.13 while paper added it back. I guess you could try to PR concurrent hashmaps into paper to try and fix this issue. I might also try to eventually.

YellowPhoenix18 commented 2 years ago

Well, you are right, that this issue should be solved, but as you stated in the last sentence: It should be included into Paper instead of reflecting everything. Reflections reduce performance and are not the best option. Slighly changes will "fuck up" everything.

And this is why we also often do not agree with Paper. Their devs always blame the plugin-devs for doing things wrong, while the plugins work fine on spigot.

Yes spigot removed the async chunk-generation, but this is no reason for Paper to make a half-working solution and tell the developers to fix their buggy code.

Lauriichan commented 2 years ago

In addition, sure you can make a PR but we will not as we're quite busy working on our own infrastructure in the background. And "fixing" something by replacing a variable with reflections isn't the right way to go. We're always trying to avoid NMS and CB as much as we can and we will stick to that decision.

Lauriichan commented 2 years ago

All our NMS and CB access is publicly accessible in this repository and it isn't much at all. We're trying to decrease that as much as we can but some things just can't be removed with our current plugin structure which we plan to change in the future. All though we don't know how much NMS and CB access we can prevent as well as using reflections for things

Phoenix616 commented 2 years ago

Just fyi: Even the Spigot-API requires the generation methods to be thread safe: https://hub.spigotmc.org/javadocs/spigot/org/bukkit/generator/ChunkGenerator.html (Most likely to be able to maybe move to async generation at some point in the future?)

So you really shouldn't call any plugin API methods that aren't clearly marked as thread safe in your generator unless you ensured that you are on the main thread beforehand. And the error is not a CB/Spigot/Paper/Tuinity internal bug as the API method wasn't marked as thread safe to begin with.

YellowPhoenix18 commented 2 years ago

@Phoenix616 As far as I know Bukkit.createChunkData was designed for the ChunkGenerator. The method in the chunkgenerator was working in 1.13 and is included for backward-compability. Based on the old spigot-docs from 1.13 the new method createChunkdata wasn't even mentioned nor was mentioned it has to be thread-safe, which it obviously is, see here: https://helpch.at/docs/1.13.2/org/bukkit/generator/ChunkGenerator.html . The onliest option that might be the problem here could be the creation of materials, but that should be fixable

Phoenix616 commented 2 years ago

@YellowPhoenix18 This is 1.17.1 we are talking about, not 1.13.2. It doesn't matter how it was in old versions, plugins need to adapt when the underlying game and the API changes and the generateChunkData deprecation and thread-safe requirement are such changes introduced by newer versions being able to do the chunk generation off-thread.

YellowPhoenix18 commented 2 years ago

@Phoenix616 Im aware we are talking about 1.17.1 and as I mentioned we will take a look into there. We probably just missed the change with the thread-safe information and that was, what my last message should state. This can happen. We will adjust that and then its fine.

In addition the mentioned method seems to not be the problem at all, the problem here seems to be as already mentioned by @MWHunter , that material-parsing through Bukkit in the ChunkGenerator runs into issues because of the caching used.

@Lauriichan and me will take a look into it and bring in adjustments.

But I see no reason for this thread to explode into a spigot-community-talk

YellowPhoenix18 commented 2 years ago

The Issue should be resolved in total now

Lauriichan commented 2 years ago

So after investigating further more we found out that CraftBlockData.newData() is thread safe on spigot. However, on Paper this is not the case.

The CraftBlockData class on Spigot initializes a lookup cache for BlockData on class initialization while Paper doesn't do that. Instead it creates the CraftBlockData lookup cache dynamically to save RAM usage. Due to that change the method Bukkit.createBlockData() is no longer thread safe on Paper but on Spigot it is thread safe indeed.

Why isn't it Thread Safe on Paper? Because Paper uses a computeIfAbsent call to cache stuff if it is not already there while in Spigot it is guaranteed to be available and the Map is never changed.

For reference in Spigot source click here

@Phoenix616 @MWHunter

Phoenix616 commented 2 years ago

in Spigot it is guaranteed

This assumption is simply wrong. Nowhere in the API specification is it mentioned that the methods are thread safe, Spigot could change the implementation at any point without any kind of notification to devs as the API specification does not require it to be thread safe and Paper does exactly that to provide performance improvements as thread safety is not required.

Please do not fall victim to Hyrum's Law and confuse implementation behaviour with API specification. This gets you down a very dumb road which even Microsoft engineers are fighting with. (Hint: The correct solution to the mentioned Windows issue is to simply update the broken software to the changes, something that is easily possible when using open source software and not using software they can fix bugs in is their own fault and they should have to pay for it, not Microsoft or other Windows users)

The solution here is to either open a PR to Bukkit/Spigot-API to make the API specification require thread safety (and provide the proof that it already is thread safe there to get it accepted) and hope that Paper actually agrees with it, PR an alternative API with guaranteed thread safety if it can't be guaranteed by the current implementation, or simply access the Bukkit API methods on the main thread like it is suggested by literally everyone.

Lauriichan commented 2 years ago

I'm not talking about the API specification, I'm talking about the Implementation yeah And we already started an PR to Paper because of that, maybe we'll also start one to spigot to mark that in the docs but something like that needs time. And yes, there is no other fix than that. We just can't create all the BlockData on the main thread, that would slow down a lot of things in our current architecture. For example our Schematic Data is currently not BlockData but instead a selfmade class for easier BlockData manipulation which is required for the schematics and the up comming rotations