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

Refactor level flipping #1922

Merged
merged 3 commits into from
Dec 15, 2021

Conversation

mrkubax10
Copy link
Member

Fixes issues with level flipping.

@Semphriss Semphriss self-requested a review December 10, 2021 19:01
@Semphriss
Copy link
Member

Self-requested a review because I've noticed a few ways to improve the code, I'll check that in-depth and I''ll take the time to write my comments properly whenever possible (might take a few days, feel free to remind me if I seem to have forgotten).

@@ -33,20 +33,11 @@ class Decal;
/** Vertically or horizontally flip a level */
class FlipLevelTransformer final : public LevelTransformer
{
public:
static Flip transform_flip(Flip flip);
Copy link
Member

Choose a reason for hiding this comment

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

Is there any upside to use this rather than taking the flip as (non-const) reference and modifying it directly that way?

(Suggesting that option as it would avoid an extra assignment in the code from callers)

void
Platform::on_flip(float height)
{
if (Path* const path = get_path()) {
Copy link
Member

Choose a reason for hiding this comment

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

You have used Path* const at a few places (I won't mark them all as I usually do), which is a const pointer to a non-const object; is there a particular reason to do it this way?

@tobbi
Copy link
Member

tobbi commented Dec 15, 2021

Please rebase!

@tobbi tobbi added status:needs-work In progress, but no one is currently working on it (New volunteers welcome) and removed status:needs-work In progress, but no one is currently working on it (New volunteers welcome) labels Dec 15, 2021
@mrkubax10
Copy link
Member Author

I will do that as soon as possible

@mrkubax10 mrkubax10 force-pushed the fix_unstable_tiles_at_wrong_position branch from a63a6fd to 529c9e8 Compare December 15, 2021 15:12
@tobbi tobbi merged commit b61b1ce into SuperTux:master Dec 15, 2021
@mrkubax10 mrkubax10 deleted the fix_unstable_tiles_at_wrong_position branch December 16, 2021 08:19
mrkubax10 added a commit to mrkubax10/supertux that referenced this pull request Dec 18, 2021
tobbi pushed a commit that referenced this pull request Dec 18, 2021
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.

3 participants