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

Fix getMaxHeight to be exclusive instead of inclusive to match docs #11675

Closed
wants to merge 1 commit into from

Conversation

MrPowerGamerBR
Copy link
Contributor

@MrPowerGamerBR MrPowerGamerBR commented Nov 27, 2024

The getMaxHeight docs says that the value is exclusive, not inclusive. This has changed in 1.21.3 and since 1.21.3 the value is now inclusive due to Mojang changes. This patch changes the value to be exclusive by +1 the returned value from NMS.

Worth noting that this needs to be tested! I haven't tested it yet, and I seen that there are some CraftBukkit code that does call getMaxHeight (setBiome)

Fixes #11674

@MrPowerGamerBR MrPowerGamerBR requested a review from a team as a code owner November 27, 2024 05:28
@Lulu13022002
Copy link
Contributor

Lulu13022002 commented Nov 27, 2024

This is not the only method affected:
Chunk#getBlock/getChunkSnapshot are now inclusive, nvm got confused by what you called exclusive compared to the doc
Chunk#getEmptyChunkSnapshot throw an ArrayIndexOutOfBoundsException with your changes (at y = getMaxHeight) instead of the IllegalArgumentException

@NonSwag
Copy link
Contributor

NonSwag commented Nov 28, 2024

Wouldn't it be smarter to adjust the docs instead of the method?

@electronicboy
Copy link
Member

No, the API docs describe the contract of the method, there is no sane justification for changing the API contract here

@MrPowerGamerBR
Copy link
Contributor Author

MrPowerGamerBR commented Nov 28, 2024

This is not the only method affected: Chunk#getBlock/getChunkSnapshot are now inclusive, nvm got confused by what you called exclusive compared to the doc Chunk#getEmptyChunkSnapshot throw an ArrayIndexOutOfBoundsException with your changes (at y = getMaxHeight) instead of the IllegalArgumentException

Looks like I ended up checking only getMaxHeight usages on the API module, not on the server...

Well now this is a bit trickier, I don't know if I should assume that all getMaxHeight on the server are assuming that the value will be inclusive (new docs) or exclusive (old docs).

Wouldn't it be smarter to adjust the docs instead of the method?

Plugins already rely on the behavior of the current docs, I found this issue because my plugin stopped checking for the block at the top of the world due to this, so I dug deeper to find what was causing the issue and, while I found out that checking world.minHeight..world.maxHeight did fix the issue, that's not what I expected according to the docs and what I was expecting when I coded the plugin years ago. Other plugins also have issues due to this change, like mcMMO.

for (y in world.minHeight until world.maxHeight) {
   ...
}

@jacob1
Copy link

jacob1 commented Nov 30, 2024

I ran into this same issue while updating Dynmap. I reported it to Spigot and it's fixed upstream now.

Looks like an identical patch to this one. Any potential issues included.

@Machine-Maker
Copy link
Member

I think the issue also exists for WorldInfo#getMaxHeight

@Lulu13022002
Copy link
Contributor

I just checked all the usages and the issue has been fixed in 1.21.4 (technically latest commit of 1.21.3 with paper fixes), thanks for the report regardless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

getMaxHeight() has incorrect behavior since 1.21.3
6 participants