Skip to content

aabb2d_zero, aabb2d_diagonal and aabb2d_size #392

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

Merged
merged 5 commits into from
Feb 24, 2024
Merged

Conversation

duarm
Copy link
Contributor

@duarm duarm commented Feb 19, 2024

  • adds _aabb2d_zero.

Problem

Currently aabb2d_size returns vec2_distance(min, max), which would be the diagonal of the aabb, if you want to use the size to resolve collisions, you need to work backwards from what aabb2d_size does when it calls vec2_distance.

Solution

  • renames _size -> _diag. And provide a macro for compatibility
  • adds a _sizev function which outputs a vec2, which is basically glm_vec2_sub(max, min, out).

image

Looking for feedback if this is desirable, maybe come up with another name to avoid a breaking change on _size?

@duarm
Copy link
Contributor Author

duarm commented Feb 19, 2024

I should also split _zero into another PR

@recp
Copy link
Owner

recp commented Feb 22, 2024

Hi @duarm,

Sorry for the delay, I like the your new interpretation of _size and _diagonal/_diag we can go in this way. This will break existing codebase so keeping and deprecating _size for backward compatibility, introduce _sizev ( v/vector suffix ) for sizes as vector and _diagonal/_diag for size of diagonal would be an option.

Feedbacks are welcome

@duarm
Copy link
Contributor Author

duarm commented Feb 23, 2024

size -> diag and new _sizev function, made a macro for _size to _diag. Should we do the same for aabb, or should I open another PR?

@duarm duarm marked this pull request as ready for review February 23, 2024 17:07
@recp
Copy link
Owner

recp commented Feb 24, 2024

Should we do the same for aabb, or should I open another PR?

Yes other PR would be better, thanks 👍

@recp recp merged commit b7e4c96 into recp:master Feb 24, 2024
@recp
Copy link
Owner

recp commented Feb 24, 2024

@duarm the PR is merged, thanks for your contributions 🚀

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.

2 participants