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 new flags to minetest.features for 5.8.0 features #13978

Conversation

Desour
Copy link
Member

@Desour Desour commented Nov 9, 2023

#12764 added the start_time sound param. The server needs to read this param and send it to the clients. Mods need this feature flag to check if the server supports this.

(Client support can only be checked via protocol version. AFAIK, it's like this for every feature, so this is fine.)

To do

This PR is a Ready for Review.

How to test

Read.

@Desour Desour added @ Documentation Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines labels Nov 9, 2023
@Desour Desour added this to the 5.8.0 milestone Nov 9, 2023
Copy link
Member

@grorp grorp left a comment

Choose a reason for hiding this comment

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

LGTM

@Wuzzy2
Copy link
Contributor

Wuzzy2 commented Nov 9, 2023

While you're at it, can you please also add one entry for the new physics override in player:set_physics_override? Thx.

@Desour
Copy link
Member Author

Desour commented Nov 9, 2023

While you're at it, can you please also add one entry for the new physics override in player:set_physics_override? Thx.

You're referring to #11465, right? There are multiple new fields, do you have a suggestion for the flag name?

@Wuzzy2
Copy link
Contributor

Wuzzy2 commented Nov 10, 2023

Idk, maybe new_physics_overrides?

builtin/game/features.lua Outdated Show resolved Hide resolved
@Desour Desour changed the title Add minetest.features.sound_parameter_table_has_start_time Add new flags to minetest.features for 5.8.0 features Nov 10, 2023
@Desour
Copy link
Member Author

Desour commented Nov 10, 2023

Done.

@srifqi
Copy link
Member

srifqi commented Nov 10, 2023

Since the sound parameter table's start_time has the "Available since feature ..." comment, should we also add that to the set_physics_override's override_table?

Example
diff --git a/doc/lua_api.md b/doc/lua_api.md
index 85a6cc559..663c47d90 100644
--- a/doc/lua_api.md
+++ b/doc/lua_api.md
@@ -7730,25 +7730,31 @@ child will follow movement and rotation of that bone.
         * `gravity`: multiplier to default gravity value (default: `1`)
         * `speed_climb`: multiplier to default climb speed value (default: `1`)
             * Note: The actual climb speed is the product of `speed` and `speed_climb`
+            * Available since feature `new_physics_overrides`.
         * `speed_crouch`: multiplier to default sneak speed value (default: `1`)
             * Note: The actual sneak speed is the product of `speed` and `speed_crouch`
+            * Available since feature `new_physics_overrides`.
         * `liquid_fluidity`: multiplier to liquid movement resistance value
           (for nodes with `liquid_move_physics`); the higher this value, the lower the
           resistance to movement. At `math.huge`, the resistance is zero and you can
           move through any liquid like air. (default: `1`)
             * Warning: Values below 1 are currently unsupported.
+            * Available since feature `new_physics_overrides`.
         * `liquid_fluidity_smooth`: multiplier to default maximum liquid resistance value
           (for nodes with `liquid_move_physics`); controls deceleration when entering
           node at high speed. At higher values you come to a halt more quickly
           (default: `1`)
         * `liquid_sink`: multiplier to default liquid sinking speed value;
           (for nodes with `liquid_move_physics`) (default: `1`)
+            * Available since feature `new_physics_overrides`.
         * `acceleration_default`: multiplier to horizontal and vertical acceleration
           on ground or when climbing (default: `1`)
             * Note: The actual acceleration is the product of `speed` and `acceleration_default`
+            * Available since feature `new_physics_overrides`.
         * `acceleration_air`: multiplier to acceleration
           when jumping or falling (default: `1`)
             * Note: The actual acceleration is the product of `speed` and `acceleration_air`
+            * Available since feature `new_physics_overrides`.
         * `sneak`: whether player can sneak (default: `true`)
         * `sneak_glitch`: whether player can use the new move code replications
           of the old sneak side-effects: sneak ladders and 2 node sneak jump

The only other feature that has this kind of comment is formspec_version[<version>] (feature formspec_version_element, 5.1.0).

@Zughy
Copy link
Member

Zughy commented Nov 10, 2023

@srifqi that definitely helps. The other day it took me 30 minutes to understand why start_time wasn't working, to then manually verify through git blame when it was added (to then find out it wasn't available in 5.7).

However, we can apply this logic to every new function and argument, and the docs would become a mess in no time

@srifqi
Copy link
Member

srifqi commented Nov 11, 2023

However, we can apply this logic to every new function and argument, and the docs would become a mess in no time

Other ways to document these are

  • using simpler label/description, e.g. "(since 5.8.0)", see cppreference.com, Python docs, and NumPy docs; or
  • removing all references (except in feature flags if any), see Godot docs, Unreal Engine docs, and Blender docs; or
  • only adding comments for current version, i.e. only new features in 5.8.0, and removing comments for features added in older versions.

@Desour
Copy link
Member Author

Desour commented Nov 11, 2023

I wasn't sure if I should add such a note for set_physics_override too, but either I'd have to add it to every field (which would be a bit redundant to read), or add a single note saying some of the fields are available since that flag (horrible, because one wouldn't know what fields this is even talking about). In the end I decided against it.

We usually don't add such notes, afaik. I've just added it to start_time because I found it neat there, and because I wasn't sure. Should I remove it again?
(Considering that the sound doc has changed significantly, comparing to old doc version might be tricky, so I would actually tend to like to keep it because of this.)

@srifqi that definitely helps. The other day it took me 30 minutes to understand why start_time wasn't working, to then manually verify through git blame when it was added (to then find out it wasn't available in 5.7).

AFAIK, the feature flags are not to intended to communicate such differences in the API. They are rather to make it possible for mods to check in code whether they are using the one or the other API version. (Only required in cases where the new API doesn't quack differently than the old API. 🦆)

doc/lua_api.md Outdated Show resolved Hide resolved
Copy link
Member

@srifqi srifqi left a comment

Choose a reason for hiding this comment

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

This PR looks good to me.

builtin/game/features.lua Outdated Show resolved Hide resolved
@sfan5 sfan5 added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Nov 12, 2023
@srifqi srifqi removed the Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines label Nov 12, 2023
Also, looks like I somehow removed `liquid_fluidity_smooth, liquid_sink` before... oops
@Desour Desour removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Nov 12, 2023
@SmallJoker SmallJoker merged commit 8cf76e0 into minetest:master Nov 12, 2023
2 checks passed
@Desour Desour deleted the sounds_feature_sound_parameter_table_has_start_time branch November 13, 2023 16:47
cx384 pushed a commit to cx384/minetest that referenced this pull request Dec 9, 2023
kawogi pushed a commit to kawogi/minetest that referenced this pull request Dec 19, 2023
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

7 participants