Weird crash when using fill bucket

[OverloadedOrama]

I've noticed a very weird bug that leads to a crush when using the fill bucket tool. Steps to reproduce:

1. Select the bucket tool
2. Change the selected color
3. Add it to the preset colors (the palette)
4. Change the color again
5. Click on the canvas to fill
6. The pixels get filled as normal, but Pixelorama crashes.

I haven't discovered why this happens, and it doesn't seem like a problem with my code, because the colors get filled, which means that it's not the flood fill algorithm's fault. This also means that the frame gets drawn, but the next one leads to a crush. I'm starting to think whether it's a problem with Godot and the Color Picker itself, but if any of you has any other ideas, let me know!

[Nukiloco]

I have worked on a graphics editor plugin before on Godot. Most of the time a crash like this would have to do with something in gdscript looping constantly. Did you try debugging it by making a print statement in the bucket fill function?

[Calinou]

Remember that's there's a stack limit in functions, so you need to make sure recursive functions don't run too deep. The default call stack limit is 1024 (it can be adjusted in the Project Settings).

[OverloadedOrama]

I have worked on a graphics editor plugin before on Godot. Most of the time a crash like this would have to do with something in gdscript looping constantly. Did you try debugging it by making a print statement in the bucket fill function?

I did try and do that, it prints as it should. The bucket fill function is executing as it should, otherwise it would crash before it gets filled with color. Which means, the crash happens after the function is executed, therefore, the problem is not in the function itself. Which is exactly why this is such a weird issue.

[Nukiloco]

Maybe try to print each line that comes after calling the bucket fill function to see where it crashes.

[OverloadedOrama]

Maybe try to print each line that comes after calling the bucket fill function to see where it crashes.

I tried that too. I printed something at the end of _process and it worked. Then, I tried printing both at the end and at the start of _process. The line at the end was printed, but it stopped there, and the line at the start wasn't printed after that. This should mean that the crush happens at the start of the next frame, and not at the current frame. Right before any code manages to get executed.

[novhack]

The reason is that get_pixelv() can return a slightly different Color than which was set previously with set_pixelv() on the same position. It is probably caused by some float imprecision when Color is stored into Image. The flood_fill() algorithm then gets stuck in an infinite loop while setting, reading and comparing colors in a cycle. A secondary reason of the bug is that mouse inputs are handled in a process function which calls flood_fill() multiple times while the mouse button is being hold. In the example the first call changes correctly the color to green but immediately calls flood_fill() again to change a green to green and that is where it gets stuck. I would suggest to move mouse input handling into _input() or _gui_input() function. I tried to solve it by adding a 2D dictionary with already visited pixels so they are not visited again. It makes it a little bit slower but mostly on instances where it would otherwise get stuck. Maybe a different algorithm could solve the issue better otherwise I will make a pull request.

[OverloadedOrama]

The reason is that get_pixelv() can return a slightly different Color than which was set previously with set_pixelv() on the same position. It is probably caused by some float imprecision when Color is stored into Image. The flood_fill() algorithm then gets stuck in an infinite loop while setting, reading and comparing colors in a cycle.

This is very interesting, I didn't know that. However, if that was the case, shouldn't the crash happen all the time? As far as I have noticed, it seems to crash only when I'm doing something with the color picker.

Another weird thing I noticed was that, if I printed values inside the fill loop, the filling would obviously take longer because of all the printing, but I could not replicate the crash. It's like, if I make the algorithm run slower, the crush doesn't occur.

A secondary reason of the bug is that mouse inputs are handled in a process function which calls flood_fill() multiple times while the mouse button is being hold. In the example the first call changes correctly the color to green but immediately calls flood_fill() again to change a green to green and that is where it gets stuck. I would suggest to move mouse input handling into _input() or _gui_input() function.

This is also very interesting, but I do not think this is the problem. The flood_fill() method returns if the color is the same, so even if it gets called multiple times while the mouse button is being hold, no problem occurs because the loop doesn't get executed. I tried printing a value at the beginning of the flood_fill() method, and replicated the crash. The value only got printed once, which means the method doesn't get executed more than once.

I tried to solve it by adding a 2D dictionary with already visited pixels so they are not visited again. It makes it a little bit slower but mostly on instances where it would otherwise get stuck. Maybe a different algorithm could solve the issue better otherwise I will make a pull request.

That being said, I appreciate the help, and if you have improvements to make of any kind, feel free to make pull requests!

[novhack]

The "fix" I made solves the causality of the bug but doesn't solve the cause. It also makes the flood fill algorithm even slower. I will rather try to figure out why it happens only when a color is added to palette in the following days.

[azagaya]

I've noticed that if you pause the execution when the program freezes, it says q is [null].

[azagaya]

I made a PR to a possible work arround.. idk if a good solution but may be it can led you to the cause of the problem.

[OverloadedOrama]

This is getting even weirder. I tried implementing your PR but it didn't seem to solve anything, so I will not merge it for now. However, this discovery might be important. I realized that, when the crash happens and you pause, then unpause, then pause over and over, q is always null, but n is changing.

Could this mean that the loop is happening even after the frame ends? We know for a fact that the loop does actually end, because sprite_changed_this_frame becomes true, and at the end of _process(), update_texture() gets called. Which means that we exit flood_fill() first, right?

We also know that the method isn't called again in the next frame because if you print something at the top of flood_fill(), it only gets printed once, during the first frame, which gets completed. If it was to be called again, it would have been printed again.

It also doesn't make sense that q is null. q starts with mouse's position, and appends more pixels as it goes. How is it possible that it becomes null, exactly?

Unless there's something else going on and I just can't see it. Am I wrong somewhere?

[azagaya]

Wow, it definitely works in my laptop. But you are right.. i tried in another pc and doesn't work. This is so strange.

[OverloadedOrama]

The crash occurs on my end even if it's maximized, or not maximized. :/

[azagaya]

Hey, reading again some comments, i think its true that the difference in colors makes the loop never ends. And that difference apparently just happens when doing that steps you describe at the start... would you try if adding this helps?

for n in q:
  var c = target_color - replace_color
  var dist = sqrt(c.r*c.r + c.g*c.g + c.b*c.b + c.a*c.a)
  if dist <= 0.005: break
  #rest of the loop

[OverloadedOrama]

I added it, and it appears to be working! I'm not entirely sure if this solves the main cause on the bug, because while I was going crazy with it, I did manage to have it crash on me again, but I can't seem to replicate it. It may be because I also was printing something. But this piece of code definitely helped!

I'm still not sure why this happens, though. I mean, I understand that there is a difference in colors, but, if the method was getting executed again, why didn't it print anything again? I guess it's possible that crashed before it managed to print?

Also, there is still another, but rather minor problem. With Raw Mode enabled, if you have color values that are larger than one and try to fill, Pixelorama crashes. I consider it minor because I doubt that many people actually use raw mode, but if anyone would like to give it a shot in finding the problem, go ahead!

Thank you all for taking the time to reply and help find the root of this issue. I'm going to leave this issue open for a little while longer if anyone wants to contribute anything extra or finds another cause of a crash. If there's nothing, I will close it after some days.

[azagaya]

Hi! Im glad to help,but this is not a proper solution.. it will only work arround if the distance between the colors is 1 in 255.. and sometimes if the distance is bigger it crashes again, and it happened to me twice. I proposed that just to test if that could help us find a real solution. Its strange that that values are near each other in first place because when i clicked all was transparent.

[OverloadedOrama]

Oh okay I see, then I won't close the issue. This whole thing is very confusing, but at least we are getting somewhere! I'll try to see if I'll come up with something else as well.

[OverloadedOrama]

The crash hasn't occurred to me ever since #31 was merged, and I haven't seen anyone else encountering this issue, so I think I'll finally close it. I don't know if the actual problem is fixed, but it looks like the crash isn't happening anymore.