fill tool crash - Githubissues

oceank2 commented 8 years ago

Hi, I already report this bug. When I try to fill in a big region with the costumes editor, the tool crashes. I tested it with Firefox and Google Chrome. Can somebody look at this problem. Thanks

towerofnix commented 8 years ago

IMO creating a new issue isn't the best way to bring up a topic again - maybe just add a friendly "has any work been done on this?" to the original issue, #1170?

cycomachead commented 8 years ago

IMO creating a new issue isn't the best way to bring up a topic again - maybe just add a friendly "has any work been done on this?" to the original issue, #1170?

Yes, that's probably the best way. Unfortunately, I don't think anyone has had time to take this on; sorry!

oceank2 commented 8 years ago

If it can help, the fill block of the pen tab works nice

jmoenig commented 8 years ago

yeah, but for that fill primitive I basically copied @Hardmath123 's flood fill code from the paint editor :-)

jguille2 commented 7 years ago

Hi! First, I think we must close #1170 because this is the same issue.

My teachers have reported me the same issue. I've investigated and I think I've seen where is the problem.

The problem appears depending on the working language. I can reproduce the problem (fill color feature crashed in costume editor) in Catalan and French. In English is running OK. Maybe there are browser differences (we'll see it depends on the visualization) but there is a really language dependance.
Where is the language difference? In the "Constrain" ToggleMorph labelString. See Catalan and French strings.
There are larger than English one... and this causes the problem.
The problem is not in the string length, because we can use more than one line. The problem is in the size of each line. If one line is "too" large, it crashs.
There is not a clear limit of number of characters. It's around 40... but the problem is in the size displayed (different characters, different size).

Sure, we can fix it changing string translations, but maybe we can fix it better. I'm sorry but I have not been able to fix it (I have tried it, but the editor was a little confused for me). crashinglimit

Thanks, Joan

bromagosa commented 7 years ago

I'm sure this has something to do with the crash because you're right in that it always crashes in those cases, but it also crashes anytime on Android, in English, so there must definitely be something else to it :)

bromagosa commented 7 years ago

Here's what happens in Android: https://youtu.be/5QTWE42nBBA

jguille2 commented 7 years ago

Hi @bromagosa, But maybe problem is the same, the size (displayed) of this string... and depends on the browser-SO.

I would want to test your case with a shorter string (shorter than English one). Maybe I'm wrong. If a shorter string is still KO, I would find in another editor tool (buttons, colorpicker...). It's like when some element is bigger, it deforms the visualization... and the algorithm for calculating the area to be colored crashs.

bromagosa commented 7 years ago

Crashes in Chinese too, and it doesn't get shorter than that :stuck_out_tongue_winking_eye:

cycomachead commented 7 years ago

Hmm, I'm not sure why the language makes a difference, but that's a very helpful tip because it means more of us can reproduce this easily.

bromagosa commented 7 years ago

Yep, I'm just saying the language is not the root cause, but a trigger for the root cause in certain browser/os configs.

In Android it crashes in all languages, in (my) Firefox it crashes independently of the language, and in (my) Chrome it did crash before in Catalan and French, but now it doesn't crash any more:

out

More insight: in Firefox I went into dev mode, removed the "Constrain proportions" TextMorph, called fixLayout() for the PaintEditorMorph, and the issue was gone. So it definitely has to do with the layout of the editor.

An quick and ugly fix would be to restrict the width of that TextMorph. A better fix would be to... well, fix the editor.

bromagosa commented 7 years ago

More findings: it seems to crash because, at PaintCanvasMorph.floodfill, the stack keeps growing indefinitely and it never shrinks to length 0.

I can't wrap my head around what does this have to do with the layout.

jmoenig commented 7 years ago

roping Kartik - @Hardmath123 - into this discussion...

cycomachead commented 7 years ago

More findings: it seems to crash because, at PaintCanvasMorph.floodfill, the stack keeps growing indefinitely and it never shrinks to length 0.

That was my initial guess. Then you mentioned the layout, and I thought I was just stupid.

There's an infinite loop since otherwise I'd expect something to be logged. I've been reading morphic for the past hour and I don't see how calling fixLayout() which calls fullBounds() on a few morphs would lead to infinite loops.

I'm still confused....

bromagosa commented 7 years ago

@cycomachead I think I led you to mix things up, sorry!

What I meant are two different things:

1) For some reason, removing that TextMorph and calling fixLayout() on the PaintEditorMorph fixes the issue, but I'm quite sure that's a side effect of the real root cause. 2) The stack I'm talking about is the stack variable in the floodfill function. I console-logged its size at each iteration (inside the while (stack.length > 0) { loop) and it seems to grow indefinitely.

cycomachead commented 7 years ago

Haha, you made sense! I just meant that the combination of those two is very very odd.

bromagosa commented 7 years ago

Oh I see! It is odd indeed, and I just found out something else:

On a non-crashing instance, if you console-log the width of the paper it prints out 480, whereas on a crashing instance it prints out 479. But here's the funny part: if you inspect it in dev mode, its width is definitely 480.

My scientific diagnose is that there's some kind of black magic sorcery stealing a pixel away from the paper before it gets to floodfill().

And here I thought this would be a 5 minute fix to start off the day...

bromagosa commented 7 years ago

...which seems to have nothing to do with the issue. Fixing the dimensions solves nothing :(

bromagosa commented 7 years ago

GOT IT! And it did had to do with a rounding error in the dimensions.

I'm issuing a pull request right away :)

jmoenig commented 7 years ago

Yay!! thank you, Bernat! Terrific catch!!

jguille2 commented 7 years ago

@bormagosa, thank you very much! I feel sorry for the time spent ... but if you had done in only five minutes, my self-esteem would be on the ground. :sweat_smile:

kach commented 7 years ago

Yikes. Thanks for figuring this out, Bernat, that's entirely on me being careless with rounding. :-/

bromagosa commented 7 years ago

Nah @Hardmath123, that's on JS for not having integers. As much as I like JS for its good parts, I believe this is one of its indefensible WATs.

Other WATs are bypassable if you stay on the safe side and listen to Crockford, but this one is going to bite you sooner or later no matter what you do :(

brianharvey commented 7 years ago

@bromagosa If you talk Jens into using the Scheme numeric tower library in Morphic, then we won't have such problems... :P

jmoenig commented 7 years ago

Well, @brianharvey , there actually is a potentially fatal problem with using the "infinite precision library" in Snap! projects: It will make everything infinitely precise in Morphic, including the coordinates, scales, rotations of scripts, blocks, sprites, even when you save such projects to disc or to the cloud. Since many of these properties are, in fact, serialized within the project this could very well blow up project sizes to problematic levels.

brianharvey commented 7 years ago

Oh, I know, I was just jerking your chain. That's what the ":P" means!

I think, though, that you're exaggerating the problem a bit. Rotations should all be kept mod 360. Almost all the morphs are constrained to stay in the window; it's only if you move a sprite very far offstage that huge numbers would appear in Morphic.

jmoenig commented 7 years ago

Sure, I noticed the ":P", but my point is actually a serious one: This isn't about bignums but about infinite precision resulting in very many post-decimal point digits that may all be turned into strings upon serialization...

cycomachead commented 7 years ago

I don't think you typical serialize precise numbers as decimals - you can represent them as fractions.

jmoenig commented 7 years ago

Sure, but who knows what Brian's library does, and whether Snap is able to deserialize such a project in a situation where that library has not yet been installed in Snap?

All I'm saying is, be careful with libraries that change the way JavaScript works...

brianharvey commented 7 years ago

who knows what Brian's library does

Hmpf. You could find out just by doing an experiment: flonum

It's only integers that are infinite-precision (and, as Michael says, the numerator and denominator of exact rationals).

The underlying Javascript library has a feature that lets you print an exact rational as a decimal fraction to a precision that you specify, but it does so by explicitly generating a string; inexact values (such as come from transcendental functions) are represented as plain Javascript numbers.

And I don't see how it's possible to have a project that contains a bignum but not the bignum library.

cycomachead commented 7 years ago

We could write a "wrapper" that would deserialize Bignums / exact values to JS numbers when the library isn't present and then load the library when necessary. It would still be a decent change to Snap! to support things natively.

My point was simply that it's possible to integrate this in a safe way. No one is advocating integrating 3rd party code in a willy-nilly way!

-- Michael Ball From my iPhone michaelball.co

On Mar 20, 2017, at 8:51 AM, Brian Harvey notifications@github.com wrote:

library

kach commented 7 years ago

Ah, closure! This had been haunting me for the past 14 months.

It's a Morphic issue: Morph.getPixelColor() doesn't transform the alpha-byte back to a fraction.

Smells like the kind of thing types would solve ^{(he says, ducking from Jens).}

jmoenig / Snap

fill tool crash #1349