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

Add rectangular cuboid shape #883

Merged
merged 2 commits into from
Nov 21, 2020
Merged

Conversation

e00E
Copy link
Contributor

@e00E e00E commented Nov 17, 2020

Based on #377 updated to current master.

I have renamed Prism to Cuboid because this is a more common name and easier to search for.

@e00E
Copy link
Contributor Author

e00E commented Nov 17, 2020

I see that CI is currently failing on master unrelated to this PR.

@cart
Copy link
Member

cart commented Nov 18, 2020

yeah. our math library glam pushed out a minor version change that deprecated accessor methods like .x(). We've fixed that on master, so a rebase + force push should resolve the issue.

@cart
Copy link
Member

cart commented Nov 18, 2020

I think this should either be called Cuboid, RectangularPrism, or just Box (Box has my preference, but it also collides with the rust Box type, so we probably shouldn't use that). RectangularCuboid is redundant.

Lets also take this chance to correct the "cube" shape size and divide the min/max extents by 2.0. Lets just use the Cuboid constructor for simplicity.

@e00E e00E force-pushed the add-rectangular-cuboid branch 2 times, most recently from be20a44 to 4ab3925 Compare November 18, 2020 05:10
@e00E
Copy link
Contributor Author

e00E commented Nov 18, 2020

Renamed to Cuboid. Wikipedia claims a cuboid is more general than what we have which is a rectangular cuboid. Imo either name is fine.

Also changed the constructor's length, width, height to be x_length, y_length, z_length to make it clear which axis this refers to.

Lets also take this chance to correct the "cube" shape size and divide the min/max extents by 2.0. Lets just use the Cuboid constructor for simplicity.

Not sure exactly what you mean. I have now removed the old Cube because Cuboid can be used just as easily. In Cuboid the extents are already correct, right?

@Moxinilian Moxinilian added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Nov 19, 2020
@cart
Copy link
Member

cart commented Nov 21, 2020

Ah yeah wikipedia does say that "sheared" rectangular prisms are also cuboids. I think its better to roll with the Box name at this point. Its the most intuitive and namespacing can always protect against collisions. I re-added Cube because I think its a common enough shape to merit a "shortcut" that encodes intent. I also moved the shapes module to their own file for better organization.

@cart cart merged commit d458406 into bevyengine:master Nov 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants