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
Set combined orientation #14702
Set combined orientation #14702
Conversation
14f3787
to
9558514
Compare
Related to #14667. |
Can someone take another look and possibly merge? I have a few more PRs in the pipeline which depend on this one. |
inline constexpr unsigned char | ||
ReferenceCell::default_combined_face_orientation() | ||
{ | ||
return 1u; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you felt compelled in a follow-up patch to explain why this value, I would accept such a pull request. If you wanted to do that, you could also write this as 0b010
to make it even clearer that you're treating this as a three-bit bitfield.
set_combined_face_orientation(const unsigned int face, | ||
const unsigned char combined_orientation) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strictly speaking, this is of course incompatible. But I think there's zero chance anyone is going to use the existing functions (they may even be private
, but I don't even care enough to look that up :-) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are private, otherwise I would deprecate them properly.
{ | ||
new_hex->set_face_orientation(f, true); | ||
new_hex->set_face_flip(f, false); | ||
new_hex->set_face_rotation(f, false); | ||
} | ||
new_hex->set_combined_face_orientation( | ||
f, | ||
ReferenceCell::default_combined_face_orientation()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that is so much nicer!
Related to #14667 - where possible lets set the default orientation (now stored in
ReferenceCell
) and otherwise use the full flag instead of setting individual booleans.