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

physx-sys: Emit default values in FFI bindings for literal values #221

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

slvmnd
Copy link
Member

@slvmnd slvmnd commented Oct 31, 2023

Hi, this is a simple fix for #188

It generates comments for literal default values (ints, floats, ..) like this:

/// inflation = 1.01
pub fn PxActor_getWorldBounds(self_: *const PxActor, inflation: f32) -> PxBounds3;

/// inflation = 0
/// doubleSided = false
pub fn PxMeshQuery_sweep(unitDir: *const PxVec3, distance: f32, geom: *const PxGeometry, pose: *const PxTransform, triangleCount: u32, triangles: *const PxTriangle, sweepHit: *mut PxGeomSweepHit, hitFlags: PxHitFlags, cachedIndex: *const u32, inflation: f32, doubleSided: bool, queryFlags: PxGeometryQueryFlags) -> bool;

/// scaleMassProps = true
pub fn phys_PxScaleRigidActor(actor: *mut PxRigidActor, scale: f32, scaleMassProps: bool);

A totally different way of solving this would be to not care about the kind of AST node and use the range begin/end offset to read the source expression from the source code instead. But if almost all default values are simple values then this approach might be good enough.

Not really expecting you to merge this as is without changes, but at least it can serve as a starting point.

I haven't really written much rust before so let me know if there is something un-idiomatic.

@slvmnd slvmnd requested a review from tgolsson as a code owner October 31, 2023 03:44
Copy link
Member

@tgolsson tgolsson 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! I'll mull on it a bit if we want more done here, but I'm curious about one thing in the conversion to Param.

let default_value: Option<&str> = match &inn.inner[0].kind {
Item::IntegerLiteral { value, kind: _ } => Some(value),
Item::FloatingLiteral { value, kind: _ } => {
Some(if value == "1.00999999" { "1.01" } else { value })
Copy link
Member

Choose a reason for hiding this comment

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

This seems... weird to me. I don't think you could get a floating point error turning 1.01 into 1.0099999. Where does it show up?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the C++ headers the "inflation" parameter to several different functions often has the value of "1.01" - but the Clang++ json dump command prints these values as 1.00999999 - so changing it back to 1.01 better matches the original value in the C++ headers.

virtual		PxBounds3		getWorldBounds(float inflation = 1.01f) const = 0;

A more general fix would obviously be to handle any floating point value that seems to have been incorrectly formatted, but I think this was basically the only default value PhysX uses that has this error.

Copy link
Member Author

@slvmnd slvmnd Oct 31, 2023

Choose a reason for hiding this comment

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

Putting:

float f(float a=1.01f) {
	return a+a;
}

Through "clang++ -Xclang -ast-dump=json -fsyntax-only tst.cpp" gives:

          "name": "a",
          "type": {
            "qualType": "float"
          },
          "init": "c",
          "inner": [
            {
              "id": "0x7fe26608a580",
              "kind": "FloatingLiteral",
              "range": {
                "begin": {
                  "offset": 14,
                  "col": 15,
                  "tokLen": 5
                },
                "end": {
                  "offset": 14,
                  "col": 15,
                  "tokLen": 5
                }
              },
              "type": {
                "qualType": "float"
              },
              "valueCategory": "prvalue",
              "value": "1.00999999"
            }
          ]

🥴

Copy link
Contributor

Choose a reason for hiding this comment

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

It's the same value in f32, clang just does lazy job at human-formatting:

dbg!(1.01f32-1.00999999f32 == 0.0f32);
dbg!(1.01f32-1.00999999f32);
dbg!(1.01f64-1.00999999f64 == 0.0f64);
dbg!(1.01f64-1.00999999f64);
[src\main.rs:2] 1.01f32 - 1.00999999f32 == 0.0f32 = true
[src\main.rs:3] 1.01f32 - 1.00999999f32 = 0.0
[src\main.rs:4] 1.01f64 - 1.00999999f64 == 0.0f64 = false
[src\main.rs:5] 1.01f64 - 1.00999999f64 = 9.99999993922529e-9

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to parse and format it back in rust (and add a comment explaining it):

value.parse::<f32>().ok().map(|value| format!("{value}"))
[src\main.rs:2] "1.00999999".parse::<f32>().ok().map(|value| format!("{value}")) = Some(
    "1.01",
)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion @rlidwka - I have updated the pull request using format! instead.

@slvmnd
Copy link
Member Author

slvmnd commented Nov 15, 2023

One thing that needs to be considered is what to do about default values which this pull request does not currently handle, so for example:

const PxReal inflation = 0.0f,
bool doubleSided = false,
PxGeometryQueryFlags queryFlags = PxGeometryQueryFlag::eDEFAULT

Here this code will pick up the default values for inflation and doubleSided, but not for queryFlags.

Generating "PxGeometryQueryFlag::eDEFAULT" or something similar from the AST seems fairly complicated and fragile, so the best alternative for complex values is probably to read the value from the C++ source code, but that would be a fairly large change to how the generator is currently architected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants