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

Add paramtype2s for 4 horizontal rotations and 64 colors #11431

Merged
merged 4 commits into from
Sep 16, 2022

Conversation

Wuzzy2
Copy link
Contributor

@Wuzzy2 Wuzzy2 commented Jul 7, 2021

Fixes #6194. @VanessaE. @paramat. @C1ffisme. @Sokomine.

This PR adds 2 paramtype2s: 4dir and color4dir.

4dir is like facedir, but only for 4 horizontal directions: NESW. It is identical in behavior to facedir otherwise. The reason why game makers would want to use this over facedir is 1) simplicity and 2) you get 6 free bits.
It can be used for things like chests and furnaces and you don't need or want them to "flip them on the side" (like you could with facedir).

color4dir is like colorfacedir, but you get 64 colors instead of only 8.

Also, as with all color paramtype2s, the builtin functions were updated accordingly.

For details on how this works, see the commits (lua_api.txt). Documentation for facedir and wallmounted is also updated to distinguish those better from 4dir.

To do

This PR is ready for review.

Many test cases in DevTest were added which I used for testing in depth. I tested rotation (obviously), color, falling node, and all new nodes I added to DevTest. I also tested the new and modified builtin functions (but you should, too!). It all seems to work to me.

One thing I didn't test in depth is the prediction. I am not so sure if the placement prediction code is correct, please some verify the logic. I don't know how test this part. It would also help me if you have a hint for me on how I can test predition better (I think I basically need to simulate a lag or something ...).

How to test

Use DevTest and use the many 4dir nodes and the Param2 Tool to modify them. Compare with facedir param2 values 0 to 3, it MUST look the same.
Test if the nodes still look OK when falling (use Falling Node Tool).
Use DevTest and luacmd mod to test all the functions I modified.

Use cases

  • Colored and rotatable chests
  • Colored and rotatable beds
  • Colored and rotatable stairs (as long you don't also want upside-down stairs)

@rubenwardy rubenwardy added the Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand. label Jul 7, 2021
@Wuzzy2 Wuzzy2 changed the title Add paramtype2s for horizontal rotation and 64 colors Add paramtype2s for 4 horizontal rotations and 64 colors Jul 7, 2021
@sfan5 sfan5 added the Feature ✨ PRs that add or enhance a feature label Jul 10, 2021
@hecktest
Copy link
Contributor

How about calling it coloryaw since it's just around the Y axis?

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Jul 12, 2021

I don't really care either way. It depends on what others think. If enough people are for it, I change it. If not, then not.

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Jul 14, 2021

Note: This PR should have a concept approval because #6194 is "supported by core dev".

@rubenwardy rubenwardy added Concept approved Approved by a core dev: PRs welcomed! and removed Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand. labels Jul 14, 2021
@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Jan 10, 2022

Bump.

@SmallJoker SmallJoker added the Rebase needed The PR needs to be rebased by its author. label Jan 10, 2022
@SmallJoker
Copy link
Member

Rebase needed.

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Jan 10, 2022

Rebased.

@sfan5 sfan5 removed the Rebase needed The PR needs to be rebased by its author. label Jan 10, 2022
@ghost ghost self-requested a review February 20, 2022 00:51
@ghost
Copy link

ghost commented Mar 8, 2022

Starting to look at this today. Haven't seen anything too out of the ordinary, but haven't done much with it yet.
From a first look, this seems fine:
4dirs

src/client/game.cpp Outdated Show resolved Hide resolved
src/mapnode.cpp Outdated Show resolved Hide resolved
src/nodedef.cpp Outdated Show resolved Hide resolved
src/nodedef.cpp Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Mar 9, 2022

Did some further testing, including a test of round-tripping various vectors through dir_to_fourdir and fourdir_to_dir, no issues on that end.

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Mar 15, 2022

@Df458: Done.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks good to me now

@Zughy Zughy added the Rebase needed The PR needs to be rebased by its author. label May 28, 2022
@Zughy
Copy link
Member

Zughy commented May 28, 2022

@Wuzzy2 please rebase

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented May 29, 2022

@Zughy: Rebase complete!

@Wuzzy2 Wuzzy2 removed the Rebase needed The PR needs to be rebased by its author. label May 29, 2022
@x2048 x2048 self-requested a review August 29, 2022 19:45
@x2048
Copy link
Contributor

x2048 commented Aug 29, 2022

Tested and reviewed, but a rebase needed. I've pushed rebased branch here.

@Wuzzy2

@x2048 x2048 added Rebase needed The PR needs to be rebased by its author. >= Two approvals ✅ ✅ and removed One approval ✅ ◻️ labels Aug 29, 2022
@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Sep 8, 2022

Rebase done. Testing needed.

@Zughy Zughy removed the Rebase needed The PR needs to be rebased by its author. label Sep 9, 2022
@Zughy
Copy link
Member

Zughy commented Sep 9, 2022

@Wuzzy2 please either remove the label by yourself or quit from being a triager, this is not the first time it happens

@SmallJoker SmallJoker merged commit 1d04903 into minetest:master Sep 16, 2022
@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Sep 28, 2022

Follow-up PR for Minetest Game: #2992

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concept approved Approved by a core dev: PRs welcomed! Feature ✨ PRs that add or enhance a feature >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: simple color facedir
7 participants