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

Make some changes for size data #1636

Closed
wants to merge 1 commit into from

Conversation

parlough
Copy link
Contributor

@parlough parlough commented Aug 18, 2017

SpongeAPI | SpongeCommon

This PR makes some changes to SizeData as the keys were mutable bounded values but the data manipulator methods were just values. I also modified some documentation about how modifying these can not always work as intended.

Please read the common PR for more information and provide as much feedback as possible on documentation and functionality.

@parlough
Copy link
Contributor Author

parlough commented Sep 27, 2017

Oh man this is embarrassing one second

@parlough
Copy link
Contributor Author

These should be good for a final review/test with the fixes I made last night.

@parlough
Copy link
Contributor Author

Looking for considerations on changing base to width as it is known internally? Might make more sense as well.

@NeumimTo
Copy link

I had to guess that "base" is supposed to affect "width".

There seems to be an inconsistence with Entity#getBoundingBox where this method already returns AABB, but size data rely on "base" and "height" - two flaot params.

@ryantheleach
Copy link
Contributor

ryantheleach commented Dec 20, 2017

Base isn't width.

Width can be confused for width & depth, when base is used for both.

image

Docs could be written that base affects entities x and z components of the bounding box.

@liach
Copy link
Contributor

liach commented Dec 20, 2017

Can't entities just use any aabb for their shapes?

@parlough parlough added this to the Revision 8.0 milestone Jan 1, 2018
@Cybermaxke
Copy link
Contributor

@parlough Can you change the use of Floats to Doubles? This makes it more consistent with the rest of the API where almost everything uses Double.

@Cybermaxke
Copy link
Contributor

23d18da

@Cybermaxke Cybermaxke closed this Feb 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants