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

nalgebra-glm: Features to match GLM_FORCE_DEPTH_ZERO_TO_ONE and GLM_FORCE_LEFT_HANDED #505

Merged
merged 8 commits into from Dec 29, 2018

Conversation

Projects
None yet
3 participants
@nathanvoglsam
Copy link
Contributor

nathanvoglsam commented Dec 15, 2018

The C++ GLM library has some preprocessor macros to change the default handedness and depth range conventions. This is immensely helpful when using GLM for DirectX or Vulkan which use different NDC coordinate systems and hence require different projection matrices and handedness conventions to be used.

Unfortunately I found this library didn't have any matching construct yet, and hadn't implemented the functions for creating projection matrices for those other conventions.

What I have done is 1:1 port of the projection matrix generation code from the matching GLM functions (ortho_lh_no etc) and altered nalgebra-glm's existing perspective and ortho functions to wrap one of the handedness/depth range specific functions based on compilation configuration.

Care was taken to ensure that existing code should not be broken by the changes made. The default features enabled in the Cargo.toml ensure that nalgebra-glm will still produce the same results from the modified functions prior to my changes, and that the matrices generated by the new code match those created by the underlying nalgebra library (enforced by tests).

This should be a non-breaking change.

The added compile options are documented on nalgebra-glm's main page as well as on the functions which behavior they alter (where it explains what behavior it changes).

What I have added:

  • Implemented a number of previously unimplemented functions
  • Compile features that alter the default conventions of functions like project or ortho. Under C++ GLM these functions would normally wrap compile-time configured behavior with the GLM_FORCE_DEPTH_ZERO_TO_ONE and GLM_FORCE_LEFT_HANDED macros
  • An additional, non GLM compile feature (projection_y_flip) for applying an implict mat[(1,1)] *= -1 to all projection matrices when generated for better integration with Vulkan's NDC.
  • Additional documentation for the features I have added

Function Implementations

  • perspective_no
  • perspective_zo
  • perspective_rh
  • perspective_lh
  • perspective_fov
  • perspective_fov_no
  • perspective_fov_zo
  • perspective_fov_rh
  • perspective_fov_lh
  • ortho_no
  • ortho_zo
  • ortho_rh
  • ortho_lh

Compile Features

  • opengl_projection
  • vulkan_projection
  • directx_projection
  • projection_y_flip
  • left_hand_default
  • right_hand_default
  • zero_to_one_clip_default
  • negone_to_one_clip_default
@Ralith

This comment has been minimized.

Copy link
Contributor

Ralith commented Dec 15, 2018

cargo features are meant to be additive, because unrelated crates may independently require different sets of features from a common dependency. It looks like this change would produce confusing silent misbehavior if that happened.

@nathanvoglsam

This comment has been minimized.

Copy link
Contributor

nathanvoglsam commented Dec 15, 2018

That's a good point. In that case it would definitely cause problems, although this solution is the only way that I can think of that would allow configuring it with conditional compilation. I'd say then it would probably just be better to cherry pick all the function implementations I added and reset the modified functions back to use their default implementation to avoid misusing cargo features

@Ralith

This comment has been minimized.

Copy link
Contributor

Ralith commented Dec 15, 2018

Yeah, just exposing all the options seems like a reasonable idea, though I wonder if code bloat might be a problem.

@sebcrozet
Copy link
Member

sebcrozet left a comment

Thank you for this PR which is very well documented!
As suggested by your discussion with @Ralith, the cargo features should be removed, and the pre-existing functions reverted to their original behavior without those features.

Show resolved Hide resolved nalgebra-glm/src/ext/matrix_clip_space.rs Outdated
Show resolved Hide resolved nalgebra-glm/src/ext/matrix_clip_space.rs Outdated
Show resolved Hide resolved nalgebra-glm/src/ext/matrix_clip_space.rs Outdated
updating changes for response to pull request
Removed the compile configs based on cargo features and updated docs
@nathanvoglsam

This comment has been minimized.

Copy link
Contributor

nathanvoglsam commented Dec 19, 2018

I just went through and fixed all of the changes requested

I probably should've realized I was just making an identity matrix and used that, but I was porting behavior of one of GLM's mat4 constructors and didn't put two and two together at the time

All the docs referencing the compile features have been removed as they are no longer needed and all the functions I touched have had their docs updated to specify the handedness and depth range for the matrix/quaternion they produce.

It's currently set up to lean towards nalgebra/OpenGL conventions wherever possible (ortho_lh will map to ortho_lh_no, perspective_rh to perspective_rh_no, etc).

@sebcrozet sebcrozet merged commit ea933c6 into rustsim:master Dec 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sebcrozet

This comment has been minimized.

Copy link
Member

sebcrozet commented Dec 29, 2018

All good, thanks @nathanvoglsam!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment