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 SquarePrism shape into mesh #377

Conversation

TheJasonLessard
Copy link

@TheJasonLessard TheJasonLessard commented Aug 27, 2020

This commit add RectangularPrism, a cube-like shape where you can specify the size of each dimensions (x,y,z) independently. It also includes refactoring of the shape Cube to use it in order to reduce duplication of code. This commit also normalize the normal and UV values of the mesh created.

This commit add SquarePrism, a cube-like shape where you can specify the size of each dimentions (x,y,z) independently. It also include refactoring of the shape Cube to use it in order to reduce duplication of code. This commit also normalize the normal and UV values of the mesh created.
@@ -453,20 +400,117 @@ pub mod shape {
}
}
}

/// A Square prism. Just like a cube, but you can specify the dimensions.
pub struct SquarePrism {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be renamed RectangularPrism? A square prism would imply all sides are the same

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point!

impl Default for SquarePrism {
fn default() -> Self {
SquarePrism {
min_x: 0.0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In interest of keeping the translation around local (0, 0), should this be just length, width, and height, and not allow below 0?

I'm actually slightly surprised the cube shape isn't centered around (0, 0), dividing the size by 2 for the negative and positive, I'm not sure if that's intentional or not... but obviously that's nothing you changed

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change it, it's probably better if the default is centred on 0,0,0. I will also change the shape, it should be a rectangular prism by default, not a cube.

@karroffel karroffel added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Aug 28, 2020
@AngelOnFira
Copy link
Contributor

@cart could you rerun the CI jobs for this PR? (Artifact of #380)

@DJMcNab
Copy link
Member

DJMcNab commented Aug 28, 2020

How is this different to using a shape::Cube and a NonUniformScale
(Or at least, in theory - it appears that using any NonUniformScale only serves to stop the Translation working presently)

@CleanCut
Copy link
Member

Does #883 make this PR obselete?

@cart
Copy link
Member

cart commented Nov 22, 2020

Yup! I'll close this one out.

#883 actually uses @TheJasonLessard's code as a base. @e00E did "the right thing" here by working directly off of the commit from this pr (which leaves @TheJasonLessard's git account as an author), but GitHub did "the wrong thing" by not maintaining the correlation between the "commit author" and the github account. I'm annoyed with that ... attribution is important.

@TheJasonLessard if you want "github" attribution and not just "git commit" attribution, feel free to create a pr that "moves" the relevant code to a different spot in the file. That way you get the relevant lines associated with your account (and the new PR creates a paper trail back to this conversation).

@cart cart closed this Nov 22, 2020
@alice-i-cecile
Copy link
Member

@TheJasonLessard do you consent to relicensing this per #2373?

@TheJasonLessard
Copy link
Author

TheJasonLessard commented Oct 20, 2022 via email

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.

8 participants