-
Notifications
You must be signed in to change notification settings - Fork 43
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 }) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"
}
]
🥴
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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",
)
There was a problem hiding this comment.
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.
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:
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. |
Hi, this is a simple fix for #188
It generates comments for literal default values (ints, floats, ..) like this:
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.