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

pinhole.hpp compiler error with std::max #5414

Closed
rgolovanov opened this issue Mar 6, 2024 · 4 comments · Fixed by #5432
Closed

pinhole.hpp compiler error with std::max #5414

rgolovanov opened this issue Mar 6, 2024 · 4 comments · Fixed by #5432
Labels
🪳 bug Something isn't working 🌊 C++ API C or C++ logging API 🏎️ Quick Issue Can be fixed in a few hours or less
Milestone

Comments

@rgolovanov
Copy link
Contributor

Describe the bug
Compiler error pointing to std::max function in pinhole.hpp

const float focal_length_y = 0.5f / std::tan(std::max(fov_y * 0.5f, EPSILON));

Compiler errors:

[build] XXX\rerun\archetypes/pinhole.hpp(156,63): error C2589: '(': illegal token on right side of '::' (compiling source file YYY.cpp) [ZZZ.vcxproj]
[build] XXX\rerun\archetypes/pinhole.hpp(156): error C2144: syntax error: 'unknown-type' should be preceded by ')' (compiling source file YYY.cpp) [ZZZ.vcxproj]
[build] XXX\rerun\archetypes/pinhole.hpp(156,1): error C2661: 'tan': no overloaded function takes 0 arguments (compiling source file YYY.cpp) [ZZZ.vcxproj]
[build] XXX\rerun\archetypes/pinhole.hpp(156,25): error C2737: 'focal_length_y': const object must be initialized (compiling source file YYY.cpp) [ZZZ.vcxproj]
[build] XXX\rerun\archetypes/pinhole.hpp(156): error C2144: syntax error: 'unknown-type' should be preceded by ';' (compiling source file YYY.cpp) [ZZZ.vcxproj]
[build] XXX\rerun\archetypes/pinhole.hpp(156,63): error C2143: syntax error: missing ')' before '*' (compiling source file YYY.cpp) [ZZZ.vcxproj]
[build] XXX\rerun\archetypes/pinhole.hpp(156,63): error C2143: syntax error: missing ';' before '*' (compiling source file YYY.cpp) [ZZZ.vcxproj]
[build] XXX\rerun\archetypes/pinhole.hpp(156,1): error C2082: redefinition of formal parameter 'fov_y' (compiling source file YYY.cpp) [ZZZ.vcxproj]
[build] XXX\rerun\archetypes/pinhole.hpp(156,1): error C2059: syntax error: ')' (compiling source file YYY.cpp) [ZZZ.vcxproj]
[build] XXX\rerun\archetypes/pinhole.hpp(156,63): error C2100: illegal indirection (compiling source file YYY.cpp) [ZZZ.vcxproj]
[build] Build finished with exit code 1

To Reproduce
Steps to reproduce the behavior:

  1. Compile C++ project linking Rerun

Expected behavior
No compiler errors

Desktop (please complete the following information):

  • OS: Windows, VS 2019

Rerun version
0.14.1

Additional context
Workaround is manually patch this line by adding braces:

const float focal_length_y = 0.5f / std::tan((std::max)(fov_y * 0.5f, EPSILON));
@rgolovanov rgolovanov added 👀 needs triage This issue needs to be triaged by the Rerun team 🪳 bug Something isn't working labels Mar 6, 2024
@Wumpf
Copy link
Member

Wumpf commented Mar 6, 2024

The problem is that you include windows.h before including rerun. Windows has macros for min and max, imho the better solution is to add #define NOMINMAX before including windows.h.
Putting (std::max) in parenthesis is an interesting solution I haven't thought of before, we can add that in!

@Wumpf Wumpf added 🏎️ Quick Issue Can be fixed in a few hours or less 🌊 C++ API C or C++ logging API and removed 👀 needs triage This issue needs to be triaged by the Rerun team labels Mar 6, 2024
@Wumpf Wumpf added this to the 0.15 milestone Mar 6, 2024
@rgolovanov
Copy link
Contributor Author

@Wumpf you are right. It is coming from windows.h and NOMINMAX resolves the issue.

@nasteyi
Copy link

nasteyi commented Mar 7, 2024

explicitly passing a template parameter can also fix compilation
const float focal_length_y = 0.5f / std::tan(std::max<float>(fov_y * 0.5f, EPSILON));

Wumpf added a commit that referenced this issue Mar 8, 2024
…y from windows.h) (#5432)

### What

And add a test to ensure we don't run into this again!

* Fixes #5414

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using newly built examples:
[app.rerun.io](https://app.rerun.io/pr/5432/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/5432/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/5432/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/5432)
- [Docs
preview](https://rerun.io/preview/13b23da339babe01018016fb651da7d8f02e549c/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/13b23da339babe01018016fb651da7d8f02e549c/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
@Wumpf
Copy link
Member

Wumpf commented Mar 8, 2024

went with @nasteyi suggestion and added a test to validate rerun.hpp on our ci for this. Fixed in next release then!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪳 bug Something isn't working 🌊 C++ API C or C++ logging API 🏎️ Quick Issue Can be fixed in a few hours or less
Projects
None yet
3 participants