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 rotation of falling facedir nodes in some cases #12587

Merged
merged 1 commit into from
Jul 31, 2022
Merged

Fix rotation of falling facedir nodes in some cases #12587

merged 1 commit into from
Jul 31, 2022

Conversation

grorp
Copy link
Member

@grorp grorp commented Jul 22, 2022

The rotation of falling facedir nodes with param2 values of e.g. 255 and 24-31 is incorrect.

The engine calculates the facedir value by doing param2 % 32 % 24. The falling node code that calculates the facedir value only does param2 % 32 and doesn't rotate nodes where param2 % 32 > 23 at all.

The relevant code in MapNode::getFaceDir:

return (getParam2() & 0x1F) % 24;

and in falling.lua:

local fdir = node.param2 % 32
-- Get rotation from a precalculated lookup table
local euler = facedir_to_euler[fdir + 1]
if euler then
self.object:set_rotation(euler)
end

This PR changes falling.lua to calculate the facedir value in the same way as the engine. It also adds a test node to Devtest, testnodes:falling_facedir.

To do

This PR is Ready for Review.

How to test

In Devtest, place a testnodes:falling_facedir node and set its param2 to 255. See that it is rotated incorrectly while falling without the changes to falling.lua and correctly with them.

@sfan5 sfan5 added the Bugfix 🐛 PRs that fix a bug label Jul 22, 2022
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

Works.

@sfan5 sfan5 added this to the 5.6.0 milestone Jul 23, 2022
Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

None yet

3 participants