CesiumGS / cesium

An open-source JavaScript library for world-class 3D globes and maps :earth_americas:
https://cesium.com/cesiumjs/
Apache License 2.0
12.73k stars 3.45k forks source link

Browser hangs when drill-picking particle system #7254

Open FLYPoPo7 opened 5 years ago

FLYPoPo7 commented 5 years ago

Sandcastle example: Sandcastle example Browser: Chrome v70.0.3538.77 Operating System: windows 10

emackey commented 5 years ago

Yikes. The whole browser tab becomes un-responsive, just from mouse-over of the particle system.

mramato commented 5 years ago

We've seen this problem before (for other primitives). If I had to guess, the problem is that drillPick isn't hiding the particle system after it's picked and so it just goes into an infinite loop of repicking the same thing over and over again.

ggetz commented 2 years ago

Reported on the forum - https://community.cesium.com/t/drillpickfromray-will-crash-the-browser/19603/2.

ggetz commented 1 year ago

Also reported by @3DGISKing in https://github.com/CesiumGS/cesium/issues/10837.

DoubleYellowEgg commented 1 year ago

No repair this bug? @mramato @ggetz

ggetz commented 1 year ago

@DoubleYellowEgg Contributions are welcome! We'd be happy to review a PR fixing this issue.

javagl commented 1 year ago

This was brought up in the forum at https://community.cesium.com/t/no-repair-this-bug/24363

It obviously hangs in an infinite loop at https://github.com/CesiumGS/cesium/blob/9df6a6b914d240a9497ba276ccce63b173e2f867/Source/Scene/Picking.js#L502

Even without a deep understanding of what is happening in this code, what the intended behavior is, and how to achieve the intended behavior, a simple suggestion to at least prevent the browser from completely hanging up would be to pass a 'limit' to the call, as in

const limit = 100;
var pickedObjects = scene.drillPick(movement.endPosition, limit);

The "pickedObjects" will contain the same object 100 times then, but ... addressing that could be a next step.

(One could think that it shouldn't be sooo hard to fix this. And I know, the picking in CesiumJS has ... ... some complexities, and it's therefore impossible to know how hard it is to really fix this without affecting other cases of drillPick. But ... maybe I'll give it a try. One can argue about how important it is, but "killing browser tabs since 2018" seems like an issue that may be worth looking into... )

emackey commented 1 year ago

@javagl Having not looked at the modern source code, my memory from ~2019 is that the way Drill Picking works is to continue removing picked objects and picking (rendering for pick) again in that loop until nothing is left. It certainly seems reasonable that an upper limit (perhaps with some large default value, configurable) might be an easy sanity check to put here.

Also, if the loop picks an object on one iteration, removes it, and then somehow manages to pick the same object again on the very next iteration, that's a clear sign of an infinite loop (as well as a bad assumption about whether the object was really removed). That should be detected and reported as an internal error of some kind.

javagl commented 1 year ago

The limit is already built-in (but Number.MAX_VALUE by default). And the hint to add a limit (also given to the forum user) was only a workaround to prevent the most severe effect of this - namely, an infinite loop, blocking the browser, gradually filling the memory, because that's pretty 'Yikes' indeed. The underlying issue (i.e. why the same object is picked there multiple times) still has to be investigated, hoping that I can wrap my head around the picking complexities in reasonable time...

ethanchristensen01 commented 2 months ago

I discovered today that this infinite loop can happen with a variety of features if the user happens to be holding entities/primitives/collections in Vue Refs. It seems like putting something in a Proxy has the potential to break a lot of invariants.