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

Provided a DEFAULT_PHYSICS value #14767

Closed
wants to merge 2 commits into from
Closed

Provided a DEFAULT_PHYSICS value #14767

wants to merge 2 commits into from
+41 −1

Conversation

Mahoyomu
Copy link
Contributor

@Mahoyomu Mahoyomu commented Jun 20, 2024 •
edited by Zughy
Loading

  • Goal of the PR

  • As the title said

  • How does the PR work?

  • Struct PlayerPhysicsOverride already has its own default value, same as what's in API, don't need a new type declaration.So I just add one static variable to remoteplayer.h.

  • Does it resolve any reported issue?

  • Closes provide a minetest.DEFAULT_PHYSICS value #14764

This PR is Ready for Review.

How to test

Check the changed src.

@Zughy Zughy added @ Script API Feature ✨ PRs that add or enhance a feature Roadmap The change matches an item on the current roadmap. labels Jun 20, 2024
@rubenwardy
Copy link
Member

Hi, Zughy wants a new API feature with this available. Not a C++ static

@rubenwardy rubenwardy marked this pull request as draft June 20, 2024 20:57
@rubenwardy rubenwardy added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jun 20, 2024
@Mahoyomu
Copy link
Contributor Author

Mahoyomu commented Jun 20, 2024 •
edited
Loading

Hi, Zughy wants a new API feature with this available. Not a C++ static

You mean function to restore this value? Please help me with more details, or guides, I'm actually a newbie in this project

@Zughy
Copy link
Member

Zughy commented Jun 20, 2024

Something like minetest.PLAYER_MAX_HP_DEFAULT and minetest.PLAYER_MAX_BREATH_DEFAULT

@Mahoyomu
Copy link
Contributor Author

Something like minetest.PLAYER_MAX_HP_DEFAULT and minetest.PLAYER_MAX_BREATH_DEFAULT

Is it okay to put this in remoteplayer.h or player.h to implement them? If so I'll go with it

@Zughy
Copy link
Member

Zughy commented Jun 20, 2024 •
edited
Loading

I have basically zero knowledge of the engine structure itself, I'll let a dev answer

@Mahoyomu Mahoyomu force-pushed the Fix_value_DEFAULT_PHYSICS branch 3 times, most recently from f9a201e to 082a967 Compare June 21, 2024 02:36
@appgurueu
Copy link
Contributor

I see no reason why this can't just be a relatively trivial addition of a table of defaults in game/constants.lua. That's how minetest.PLAYER_MAX_HP_DEFAULT is implemented, for example. That said, I'm not quite sure how I feel about the feature request yet.

@Mahoyomu
Copy link
Contributor Author

I see no reason why this can't just be a relatively trivial addition of a table of defaults in game/constants.lua. That's how minetest.PLAYER_MAX_HP_DEFAULT is implemented, for example. That said, I'm not quite sure how I feel about the feature request yet.

Acknowledged. I'll do it

@Mahoyomu Mahoyomu marked this pull request as ready for review June 24, 2024 01:46
@Zughy
Copy link
Member

Zughy commented Jun 24, 2024

I think that having a single table with all the values inside is better than having all the separate single values

@rubenwardy
Copy link
Member

I agree

@Mahoyomu
Copy link
Contributor Author

I think that having a single table with all the values inside is better than having all the separate single values

Hey Zughy, I wanna know is it looks good to you now except for this? If there're more problems with the last commit, I wanna solve them in next commit, together with this one

@Zughy
Copy link
Member

Zughy commented Jun 25, 2024

I guess (can't test)

@Mahoyomu
Copy link
Contributor Author

I guess (can't test)

I moved those constant in a table, and after thinking I kept changes in constants.h, for PLAYER_MAX_HP_DEFAULT and PLAYER_MAX_BREATH_DEFAULT also have those definations in this file, maybe after changing value of those constants in the table, by using these definations in constants.h, mod authors can easily restore them. Do you think this is okay? If not I can just discard them too

Zughy
Zughy reviewed Jun 26, 2024
View reviewed changes
builtin/game/constants.lua

-- DEFAULT_PHYSICS constants of a player
core.DEFAULT_PHYSICS = {
["PLAYER_SPEED_DEFAULT"] =1,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
["PLAYER_SPEED_DEFAULT"] =1,
PLAYER_SPEED_DEFAULT =1,

There's no need to complicate the declaration with brackets and quotes. Same for all the other fields

Copy link
Contributor

Choose a reason for hiding this comment

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

I also consider the formatting a bit weird. There isn't really a benefit to having these "align". Just do speed = 1, etc.

Zughy
Zughy reviewed Jun 26, 2024
View reviewed changes
builtin/game/constants.lua

-- DEFAULT_PHYSICS constants of a player
core.DEFAULT_PHYSICS = {
["PLAYER_SPEED_DEFAULT"] =1,
Copy link
Member

Choose a reason for hiding this comment

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

Also, you have to use the same field names required by set_physics_override()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I'll check out later

@Zughy
Copy link
Member

Zughy commented Jun 26, 2024

Actually, I'm not a core dev so I have no idea of how the C++ part works. I'm generally pretty confused of the existence of the constants.h duplication (breath and hp as well), so I'll leave core devs answer

appgurueu
appgurueu reviewed Jun 26, 2024
View reviewed changes
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.

This also needs to be documented in doc/lua_api.md.

src/constants.h
@@ -97,6 +97,28 @@ with this program; if not, write to the Free Software Foundation, Inc.,
// Default maximal breath of a player
#define PLAYER_MAX_BREATH_DEFAULT 10


// DEFAULT_PHYSICS constants of a player
Copy link
Contributor

Choose a reason for hiding this comment

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

These C++ changes should be removed; this is dead code that doesn't have any effect on anything at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean these definations such as #define PLAYER_SPEED_DEFAULT ?

Copy link
Contributor

@appgurueu appgurueu Jun 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. These definitions are unused.

builtin/game/constants.lua

-- DEFAULT_PHYSICS constants of a player
core.DEFAULT_PHYSICS = {
["PLAYER_SPEED_DEFAULT"] =1,
Copy link
Contributor

Choose a reason for hiding this comment

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

I also consider the formatting a bit weird. There isn't really a benefit to having these "align". Just do speed = 1, etc.

@Mahoyomu Mahoyomu marked this pull request as draft June 27, 2024 02:21
@Mahoyomu Mahoyomu closed this Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@appgurueu appgurueu appgurueu left review comments

@Zughy Zughy Zughy left review comments

Assignees
No one assigned
Labels
Action / change needed Code still needs changes (PR) / more information requested (Issues) Feature ✨ PRs that add or enhance a feature Roadmap The change matches an item on the current roadmap. @ Script API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

provide a minetest.DEFAULT_PHYSICS value
4 participants
@Mahoyomu @rubenwardy @Zughy @appgurueu