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

Modified the Aabb2 class to be based on center + half vectors #136

Closed
wants to merge 1 commit into from
Closed

Modified the Aabb2 class to be based on center + half vectors #136

wants to merge 1 commit into from

Conversation

ceronman
Copy link

Modified the Aabb2 class to be based on center + half vectors instead of min + max. This makes some common operations a bit faster and avoids unnecessary allocations in certain operations.

The API remains the same, except for two methods added:

  • setMinMax
  • copyMinMax

These are useful when you need to get min/max vectors.

This branch fixes part of #82. I will push another pull request with the necessary changes for Aabb3.

@Fox32
Copy link
Contributor

Fox32 commented Apr 17, 2015

This change the behavior of the min and max getters and is a breaking change. Previously users could expect that chaning min and max using the getter is posssible, now it discards the values.

I don't think that this is a problem, but maybe it should also go into 2.0.0?

@ceronman
Copy link
Author

That's a good point. I completely overlooked that one. It doesn't seem possible to make it compatible with the new internal representation. For me, it's ok to wait until 2.0. I don't know what the plans are, if some compatibility breakage is allowed, I would like to change the transform methods to not returning this. That look like an anti pattern, cascading should be used.

Another option is to add a completely new class and keep the old one for compatibility reasons. That's what Ogre3D did when they introduced this change.

@Fox32
Copy link
Contributor

Fox32 commented Apr 17, 2015

I would like to change the transform methods to not returning this

This is exactly what is planned for 2.0.0, see #133.

I don't see a problem with changing the behavior, but a general problem with the getters all over the library. Maybe we should only provide getters to access the internal storage and use methods for everything else (like you already do for copy and setMinMax)?

@johnmccutchan
Copy link
Collaborator

This needs to be rebased

instead of min + max. This makes some common opperations a bit
faster and avoids unnecesary allocations in certain operations.

The API remains the same, except for two methods added:
 - setMinMax
 - copyMinMax

These are useful when you need to get min/max vectors.
@ceronman
Copy link
Author

Rebased!

@johnmccutchan
Copy link
Collaborator

Hi,

What is the status of this pull request?

@johnmccutchan
Copy link
Collaborator

@ceronman Please submit a new pull request when you're ready.

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.

None yet

3 participants