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 check for CCW of outer ring and CW for inner rings #2290

Merged
merged 3 commits into from Nov 17, 2023

Conversation

charlenni
Copy link
Member

This is regarding #2288.

Checks don't run, even if the resulting image looks totally the same. Could be, that there are problems, because the not solid line around, which could be slightly different from the original one.

@charlenni
Copy link
Member Author

Test don't run, because of the dashed line around. Now the direction is vice versa, so the line looks different from the original test. So it should be updated.

Copy link
Member

@pauldendulk pauldendulk left a comment

Choose a reason for hiding this comment

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

Great fix.

You also reverse the exterior ring to how it is supposed to be, good. I just wondered if this was really required because if it was then I think we would have found out earlier, but I gather from you comment '(if this is the other way around it also seems to work)', I gather that it is not. Weird the inner and outer rings are different in skia.

@pauldendulk pauldendulk merged commit bd3a3f6 into Mapsui:develop/4.1 Nov 17, 2023
4 of 6 checks passed
@charlenni
Copy link
Member Author

Skia needs this to distinguish between outer and inner ring/holes. It seems, that it is only important to have different orientation for holes than for the outer ring.

@pauldendulk
Copy link
Member

This fix is also needed for master btw

@pauldendulk pauldendulk changed the title Added check for CCW of outer ring and CW for inner rings Add check for CCW of outer ring and CW for inner rings Nov 17, 2023
@charlenni
Copy link
Member Author

Isn't it possible to do this with Git?

@pauldendulk
Copy link
Member

pauldendulk commented Nov 17, 2023

I personally do not have a good solution for this. I often just paste the changes in the the other branch. An alternative way is to cherry pick the commits you want to merge to master. This way it is clear this is the exact same change in master and develop, so that is probably better but I don't feel very comfortable working with commit hashes on the command line. What I would like is to have a way in github to say, and now I want this same PR to go to master and do the cherry picking for me. Perhaps that is actually possible, but I could not find this the last time I looked for it.

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.

None yet

2 participants