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

Item meta pointing range #14347

Merged
merged 10 commits into from Mar 17, 2024
Merged

Item meta pointing range #14347

merged 10 commits into from Mar 17, 2024

Conversation

cx384
Copy link
Contributor

@cx384 cx384 commented Feb 4, 2024

Goal of this PR

How does it work

It uses the item meta key range to determine the pointing range.

To do

This PR Ready for Review.

How to test

Start devtest, and left click with the testitems:telescope_stick to try out different ranges.

@wsor4035 wsor4035 added @ Script API Feature ✨ PRs that add or enhance a feature Concept approved Approved by a core dev: PRs welcomed! labels Feb 4, 2024
@SmallJoker SmallJoker added the Rebase needed The PR needs to be rebased by its author. label Feb 17, 2024
@Zughy
Copy link
Member

Zughy commented Feb 17, 2024

@cx384 rebase needed

@cx384
Copy link
Contributor Author

cx384 commented Feb 17, 2024

Rebase done.

@Zughy Zughy removed the Rebase needed The PR needs to be rebased by its author. label Feb 17, 2024
Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. The telescope stick works as expected, the code looks good, I only have minor points.

Regarding your note, I think that it would most probably be cleaner to pass the item def manager. There is no performance concern here. That would allow us to get rid of one parameter. The call site would also be more readable: getToolRange(selected_item.getDefinition(m_itemdef), hand_item.getDefinition(m_itemdef), selected_item, hand_item) would simplify to getToolRange(m_itemdef, hand_item, selected_item).

doc/lua_api.md Show resolved Hide resolved
games/devtest/mods/testitems/init.lua Outdated Show resolved Hide resolved
games/devtest/mods/testitems/init.lua Outdated Show resolved Hide resolved
@cx384
Copy link
Contributor Author

cx384 commented Feb 18, 2024

I applied your suggestions.

Regarding your note, I think that it would most probably be cleaner to pass the item def manager. There is no performance concern here.

Now it gets an item def manager and processPlayerInteraction calls selected_item.getDefinition multiple times. If I'm not wrong, it is just a lookup in a red black tree. So as you said, no performance concerns.

All fields stored in MetaDataRef are just strings, so it doesn't matter how they are set, but I changed it for clarification.

@appgurueu
Copy link
Contributor

All fields stored in MetaDataRef are just strings, so it doesn't matter how they are set, but I changed it for clarification.

Indeed, in practice this isn't an issue. FYI there are some ugly details though (for example, your set_string call was relying on number -> string coercion, which in turn was relying on tostring to properly format a float, which in practice only works because we force the C locale for number formatting) so I think using set_float / get_float and letting the engine handle the formatting is the cleanest option here.

@sfan5 sfan5 self-requested a review March 3, 2024 16:30
@sfan5 sfan5 added the Rebase needed The PR needs to be rebased by its author. label Mar 13, 2024
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.

lgtm otherwise

doc/lua_api.md Outdated Show resolved Hide resolved
src/client/game.cpp Outdated Show resolved Hide resolved
src/tool.cpp Outdated Show resolved Hide resolved
@cx384
Copy link
Contributor Author

cx384 commented Mar 15, 2024

Thanks for the review, I fixed everything and rebased.

@sfan5 sfan5 removed Rebase needed The PR needs to be rebased by its author. One approval ✅ ◻️ labels Mar 15, 2024
@sfan5 sfan5 merged commit 234b01a into minetest:master Mar 17, 2024
15 checks passed
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 @ Script API >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add per-ItemStack point range
6 participants