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.

Sign up for GitHub

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

Jump to bottom

Improve waypoints and add image variant #9480

Merged
merged 17 commits into from
Apr 11, 2020
Merged

Improve waypoints and add image variant #9480

merged 17 commits into from
Apr 11, 2020

Conversation

appgurueu
Copy link
Contributor

@appgurueu appgurueu commented Mar 7, 2020 •
edited by rubenwardy
Loading

Goal of the PR

Improves waypoints by supporting a precision & offset field (see previous PR which is incorporated inside this one)

Now also supports an alignment field (not only for image waypoints).

Works by storing precision inside the item field. Image waypoints resemble images + world_pos (see the API diff for details).

Closes the request for hidden distance. Also closes the one for image waypoints. Partially clo/ses the request for alignment & default center alignment.

This PR is ready for review.

Screenshot

Shows the waypoints created by /test_waypoints. The lowest one is a large wieldhand image waypoint. Waypoints are center-aligned by default.
Screenshot

How to test

Create a waypoint testmod with this init.lua. Then execute the /test_waypoints command.

@sfan5
Copy link
Member

sfan5 commented Mar 7, 2020

I said not to mix two different features in single PR didn't I?

@appgurueu
Copy link
Contributor Author

appgurueu commented Mar 7, 2020 •
edited
Loading

Yeah you did, sorry for mixing. I thought that if I wouldn't mix them I'd create merge conflicts if one was merged, so I merged the PR's into one.

@Wuzzy2
Copy link
Contributor

Wuzzy2 commented Mar 7, 2020

Merge conflicts are no excuse. Just because there are conflicts does not mean the other PR is doomed. Conflicts can be solved, and you should know how.

@appgurueu
Copy link
Contributor Author

I think one large PR is better than multiple small ones. Especially considering the fact that this one isn't even that "large" (~100 loc).

@rubenwardy
Copy link
Member

This isn't particularly large, but smaller PRs are much easier to review

@Wuzzy2
Copy link
Contributor

Wuzzy2 commented Mar 7, 2020

Anyway, the documentation looks good to me. I hope you can figure out how to merge this. I've been waiting for years to be able to get rid of the annoying distance number in waypoints. :-)

@appgurueu appgurueu changed the title Add offset & precision to waypoints, add image waypoint Waypoint improvements: Image waypoints, offset & precision (+ hiding distance) Mar 8, 2020
@appgurueu appgurueu mentioned this pull request Mar 14, 2020
Open
@appgurueu appgurueu changed the title Waypoint improvements: Image waypoints, offset & precision (+ hiding distance) Image waypoints & waypoint improvements Mar 14, 2020
@paramat paramat added @ Client / Audiovisuals Feature ✨ PRs that add or enhance a feature labels Mar 14, 2020
@appgurueu
Copy link
Contributor Author

appgurueu commented Mar 15, 2020 •
edited
Loading

I hope you can figure out how to merge this.

I don't see any problems with merging. It could be merged right now, provided there are no bugs or code quality issues found.

@rubenwardy
Copy link
Member

It could be merged right now, provided there are no bugs or code quality issues found.

We're currently in a feature freeze, so only bug fixes can be merged

nerzhul
nerzhul requested changes Mar 17, 2020
View reviewed changes
src/client/hud.cpp Outdated Show resolved Hide resolved
@appgurueu appgurueu requested a review from nerzhul March 17, 2020 08:53
nerzhul
nerzhul reviewed Apr 10, 2020
View reviewed changes
src/client/hud.cpp Show resolved Hide resolved
src/client/hud.cpp Show resolved Hide resolved
@nerzhul
Copy link
Member

nerzhul commented Apr 11, 2020

code sight, i'm fine with it

@appgurueu appgurueu requested a review from nerzhul April 11, 2020 15:33
rubenwardy
rubenwardy reviewed Apr 11, 2020
View reviewed changes
doc/client_lua_api.txt Outdated Show resolved Hide resolved
doc/client_lua_api.txt Outdated Show resolved Hide resolved
doc/client_lua_api.txt Outdated Show resolved Hide resolved
doc/client_lua_api.txt Outdated Show resolved Hide resolved
doc/client_lua_api.txt Outdated Show resolved Hide resolved
src/client/hud.cpp Outdated Show resolved Hide resolved
src/client/hud.cpp Outdated Show resolved Hide resolved
src/client/hud.cpp Show resolved Hide resolved
src/client/hud.cpp Outdated Show resolved Hide resolved
src/client/hud.cpp Show resolved Hide resolved
rubenwardy
rubenwardy approved these changes Apr 11, 2020
View reviewed changes
Copy link
Member

@rubenwardy rubenwardy left a comment

Choose a reason for hiding this comment

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

Tested

@rubenwardy rubenwardy changed the title Image waypoints & waypoint improvements Improve waypoints and add image variant Apr 11, 2020
@rubenwardy rubenwardy merged commit af2e6a6 into minetest:master Apr 11, 2020
aldum pushed a commit to banyamesterseg/minetest that referenced this pull request Apr 16, 2020
@appgurueu @aldum
Improve waypoints and add image variant (minetest#9480)
d759525
nerzhul pushed a commit to nerzhul/minetest that referenced this pull request Apr 27, 2020
@appgurueu @nerzhul
Improve waypoints and add image variant (minetest#9480)
6c44ec8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nerzhul nerzhul nerzhul approved these changes

@rubenwardy rubenwardy rubenwardy approved these changes

Assignees
No one assigned
Labels
@ Client / Audiovisuals Feature ✨ PRs that add or enhance a feature >= Two approvals ✅ ✅
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HUD image waypoints Allow waypoint HUD elements without distance display
6 participants
@appgurueu @sfan5 @Wuzzy2 @rubenwardy @nerzhul @paramat