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

Upgrade/floodfill #667

Merged

Conversation

MatteoPiovanelli
Copy link
Contributor

As discussed in #639 , this is my attempt at implementing the floodfill routine in gdscript

@MatteoPiovanelli
Copy link
Contributor Author

Of course last night I forgot to check formatting guidelines and such. I'll take care of that after work. Sorry for that.

@MatteoPiovanelli
Copy link
Contributor Author

@OverloadedOrama sorry to ping you. I think with my updates this PR should be able to pass the checks in the automated workflows now, but it needs authorization for those to run.

@OverloadedOrama
Copy link
Member

@OverloadedOrama sorry to ping you. I think with my updates this PR should be able to pass the checks in the automated workflows now, but it needs authorization for those to run.

My bad, I thought I had to authorize it once. Done!

@MatteoPiovanelli
Copy link
Contributor Author

I did more reformatting. Apparently I had been too zealous with going to new lines for function parameters, according to the parser.

@MatteoPiovanelli
Copy link
Contributor Author

Further refactoring. The linter failed on a line that the parser had me change. I hope it's good for both now.

@MatteoPiovanelli
Copy link
Contributor Author

I wish I could run the format/lint tasks on my machine, but there is an issue in gdtoolkit and their scripts error out on my pc.

@OverloadedOrama
Copy link
Member

OverloadedOrama commented Apr 2, 2022

Further refactoring. The linter failed on a line that the parser had me change. I hope it's good for both now.

I think the reason it fails is because of this bug Scony/godot-gdscript-toolkit#148. Best thing we can do for now is to add # gdlint: ignore=max-line-length in the previous line.

I wish I could run the format/lint tasks on my machine, but there is an issue in gdtoolkit and their scripts error out on my pc.

Yeah unfortunately if I remember correctly it's not working properly on Windows. If it's a problem for you, I can use gdtoolkit to fix the formatting/linting issues.

to line the parser has me set to longer than 100 chars
@MatteoPiovanelli
Copy link
Contributor Author

The code finally passes all automated checks. Let me know any further improvements you require.

@OverloadedOrama
Copy link
Member

For the most part it seems to be working well, but there's weird behavior when using flood fill with non-rectangular selections.
This is what happens when I use the bucket tool on an elliptical selection.
image

While this is the expected behavior.
image

For some reason, it seems to only be filling the areas with the same width. I have observed this behavior only with selections, if we draw an ellipse and use the bucket tool inside it, the result is what we expect.

@Variable-ind
Copy link
Contributor

I tried to change these lines in Bucket.gd and it seems to be working now...
test

@OverloadedOrama
Copy link
Member

I tried to change these lines in Bucket.gd and it seems to be working now... test

This seems to crash the app when a rectangular selection touches the bottom edge of the canvas (including Select All) and you use the bucket tool.

@MatteoPiovanelli
Copy link
Contributor Author

I'm checking now. The issue with the code in my PR is indeed on those lines. It's not correctly checking whether the segment above (or below) should be checked in cases where there is a selection area. Basically, it's testing above (or below) the left-most pixel of the segment.
I'm working on a fix.

@MatteoPiovanelli
Copy link
Contributor Author

The quick fix would be

segment.todo_above = position.y > 0
segment.todo_below = position.y < project.size.y - 1

That seems to cover it all. I'm working on something more precise.

@MatteoPiovanelli
Copy link
Contributor Author

I ended up going for that quick fix, with some comments explaining it, because it's just quicker than the alternatives I found.

Copy link
Member

@OverloadedOrama OverloadedOrama left a comment

Choose a reason for hiding this comment

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

I've tested it and found no other issues. The performance increase is definitely there, so this seems to be good to merge. Thank you!

@OverloadedOrama OverloadedOrama merged commit 6a7ee34 into Orama-Interactive:master Apr 16, 2022
@MatteoPiovanelli MatteoPiovanelli deleted the upgrade/floodfill branch April 19, 2022 20:26
OverloadedOrama added a commit that referenced this pull request May 20, 2022
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