Skip to content

Conversation

@Warriorrrr
Copy link
Member

isSunBurnTick includes a call to random, which makes this method return false for most of the time all the while the enitty is in full daylight. This new implementation partly copies that method, minus the monsters burn environment attribute check and the random call, to be more accurate and useful.

@Warriorrrr Warriorrrr requested a review from a team as a code owner January 4, 2026 11:48
@Warriorrrr Warriorrrr added type: bug Something doesn't work as it was intended to. scope: api labels Jan 4, 2026
@github-project-automation github-project-automation bot moved this to Awaiting review in Paper PR Queue Jan 4, 2026
Copy link
Contributor

@lynxplay lynxplay left a comment

Choose a reason for hiding this comment

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

Soooo, while removing the random here is fine, the method definitely wasn't just a "is somehow under daylight".
The in water check was there since its addition in 1.13.1 and poweder snow etc is carried over.
That means that really we are more checking if in sun and possibly about to burn, which also means we should be respecting bools controlled by org.bukkit.entity.AbstractSkeleton#setShouldBurnInDay, which override the method internally.

Probably best solved via bool params on the sunburn tick method?

@github-project-automation github-project-automation bot moved this from Awaiting review to Changes required in Paper PR Queue Jan 4, 2026
@Warriorrrr
Copy link
Member Author

#13491 removes the overrides the existing shouldBurnInDay api has, it would seem pretty counterintuitive to me that an entity not burning in daylight would mean that they are not considered to be in daylight by this method, that same PR adds a new method that answers the question of whether this mob would burn if it were to be exposed to daylight.

Not sure what would be best for this method, the changes I've pushed now just pass a bool to the method to skip the random, which should hopefully make this PR fine regardless of what happens to the other one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: api type: bug Something doesn't work as it was intended to.

Projects

Status: Changes required

Development

Successfully merging this pull request may close these issues.

3 participants