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

Fix OpenSimplexNoise get_image() swap axes #30424

Merged
merged 1 commit into from
Jul 2, 2020

Conversation

Raphael2048
Copy link
Contributor

fix #30356

@Raphael2048 Raphael2048 requested a review from JFonS as a code owner July 8, 2019 08:33
@JFonS JFonS added this to the 4.0 milestone Jul 8, 2019
@JFonS
Copy link
Contributor

JFonS commented Jul 8, 2019

I'm not against this change, but I want to note that it breaks compat with 3.1

@Raphael2048 Raphael2048 changed the title Fix OpenSimpleNoise get_image() swap axes Fix OpenSimplexNoise get_image() swap axes Jul 8, 2019
@Anutrix
Copy link
Contributor

Anutrix commented Jul 20, 2019

We can switch p_height and p_width instead of i,j in the for loop,right? Because i,j looks nicer than j,i. Just my opinion.

@aaronfranke
Copy link
Member

@raphael10241024 Is this still desired? If so, it needs to be rebased on the latest master branch. While there are no conflicts, rebasing is important so that reviewers can easily test the PR.

If not, abandoned pull requests will be closed in the future as announced here.

@Raphael2048
Copy link
Contributor Author

rebased

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

This fix is very simple and makes sense, currently i is the iterator for the height (Y axis) and j is the iterator for the width (X axis), so the order should be j, i.

The alternative is @Anutrix's suggestion of swapping width and height. I don't think it actually matters which is which, so I guess this depends on @JFonS's personal preference.

@akien-mga
Copy link
Member

akien-mga commented Jul 2, 2020

I also tend to prefer swapping p_width and p_height in the for loops so that we go through (X, Y) instead of (Y, X). But I also don't have a strong opinion either way, it's equivalent.

Edit: Actually let's keep it as is, i and j are used afterwards to index wd8 and changing that wouldn't necessarily be better.

11:47 <jfons> Akien: I don't get what you mean by swapping width and height. Making `i` iterate over width and `j` over height? I don't think that will work...
11:49 <jfons> I would go with what the PR does, it's not that bad. Loops usually iterate over height first to keep memory accesses contiguous and make them more cache-friendly
11:49 <Akien> Hm that's right, I missed that i and j are used afterwards

@akien-mga akien-mga merged commit f71e258 into godotengine:master Jul 2, 2020
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label May 31, 2021
@akien-mga
Copy link
Member

Cherry-picked for 3.4.

Note that this breaks compatibility. Users who want to preserve the behavior of 3.3 and earlier can swap the arguments in their call to get_image().

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label May 31, 2021
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.

Swapped Axes in OpenSimplexNoise Images
5 participants