jupyter-widgets / pythreejs

A Jupyter - Three.js bridge
https://pythreejs.readthedocs.io
Other
936 stars 185 forks source link

Directly changing multiple BufferAttribute arrays causes flickering in the renderer widget #292

Closed sirbardo closed 4 years ago

sirbardo commented 4 years ago

Hi, I am currently experiencing a problem with a mesh that is being updated multiple times in a row by changing a BufferGeometry->BufferAttribute->array property directly.

While this solution is very efficient and prevents memory leaks, for a BufferGeometry with self-referential attributes (position and index, or position and color), changing the two arrays one after another in Python causes a flicker in the change (I can see the position change before the color) and therefore kind of ruins what is almost a lag-free and perfect experience.

The code I am trying to get to work is here https://github.com/cg3hci/py3DViewer/blob/c21534389b798a645562635c7f1ac821a850023a/Py3DViewer/visualization/Drawable.py#L89 where I change two "array" properties in a row.

The result is the following: https://www.youtube.com/watch?v=de9jLX96zzA&feature=youtu.be

I am not sure if PyThreeJS exposes a way to "batch" multiple array/trait changes so that they are sent simultaneously to the frontend, which I believe is the problem.

Thanks,

Laerte

vidartf commented 4 years ago

When setting buffer attributes, you should set the entire buffer geometry .attributes dict in one go. You can copy the old dict (to prevent modifying the old one in-place), update the properties, and then set .attributes to the new dict. Does this fix your issue, or am I misunderstanding something?

sirbardo commented 4 years ago

Thank you for your answer!

I am not exactly sure what you mean by "copying the old dict": do you mean for example by using copy.deepcopy() ? Because that doesn't work (the deepcopy recursion breaks somewhere down the line, I can post the error), and a shallow copy will cause the same graphical glitch because the .array modifications would still cause the message over the communication channel (I just tried). Sorry if I am not being clear, this is the first time I am using both PyThreeJS and ThreeJS itself.

sirbardo commented 4 years ago

I want to add that I tried swapping the attribute dict with a new one anyway like this:

new_attributes = { 'position': BufferAttribute(tris, normalized = False, dynamic = True), 'color': BufferAttribute(colors, normalized = False, dynamic = True) } self.drawable_mesh.geometry.attributes = new_attributes

but this does not update the renderer widget that is already in the notebook, for some reason, while changing the arrays directly worked.

vidartf commented 4 years ago

Ah, I re-read your initial post and see that I misunderstood your initial question. There isn't really a clean way to update multiple arrays in one go. I agree this is a bit of a weakness with the current design. I think we could make a utility for syncing multiple array assignments, but I'm not sure I have the time right now. One thing you can try for now is to use an InterleavedBuffer[Attribute] for all the float based attributes, but you would still need to update the index attribute separately :-/

vidartf commented 4 years ago

I think maybe the simplest way would be to add a "pause"/"auto_render" trait (or some similar name) on the renderer, or maybe a context manager (with renderer.hold(): update_arrays())?

vidartf commented 4 years ago

I opened a PR that I think will fix this (untested): https://github.com/jupyter-widgets/pythreejs/pull/293

sirbardo commented 4 years ago

I think maybe the simplest way would be to add a "pause"/"auto_render" trait (or some similar name) on the renderer, or maybe a context manager (with renderer.hold(): update_arrays())?

Yeah, that was my idea too, thanks for the PR, I'll try it as soon as I can and let you know.

sirbardo commented 4 years ago

Hey @vidartf, I had a chance to test your PR but sadly even when pause_autorender is true it still autorenders.

This is what I tried:

with viewer.renderer.hold(): mesh.set_clipping(min_z = .3) # This changes the visible mesh, and causes a re-render sleep(10)

To make sure I was actually setting the variable, I added two prints in your hold function and I am 100% sure that when I make the change, the flag _pause_autorender is still True, yet it autorenders.

I still instantly see the update in the widget (and therefore the slight color/position unsyncing that causes the flickering effect).

I'll try the InterleavedBuffer approach, but I am having some trouble finding proper documentation online on how to actually use them. I'll report back. Thanks again for your time!

vidartf commented 4 years ago

@sirbardo Thanks for checking. It seemed I was missing on code path (setting the _pause_autorender caused a re-render to be queued, but that ended up being rendered after some of the changes happend. It would only render that one frame, but that should now be fixed too).

sirbardo commented 4 years ago

Thanks again @vidartf, but it's still not working. With the code from my previous comment, the behavior is unchanged. Once again, I checked that my changes happen when your flag is "true" (so the renderer should be paused") by setting up prints in your "hold" function, and I am 100% sure that it's still updating. I can build a small piece of code to reproduce the problem if needed. I'd take a look at the JS code myself but sadly I am not exactly sure how the whole "auto-generation" works with the js code.

In the meantime, I tried the interleaved buffer approach and it does work (I put color and position in the same interleaved array, and now they are synchronized). The main problem I am still having is that I also draw some LineSegments in my scene (we are building a library to prototype volumetric meshes, so we need to create an ad-hoc wireframe to show quads instead of the underlying tris), and that still lags behind if I have no way of synchronizing the updates.

I wish I could somehow contribute more. I'll try to understand how pythreejs works better so that maybe I can help add this feature.

sirbardo commented 4 years ago

BTW, here's a video showing the code causing the problem, and the problem itself. Hope it helps:

https://www.youtube.com/watch?v=Qwd99D3Wung

sirbardo commented 4 years ago

@vidartf I want to provide an update.

I experimented a bit more, and I realized that your code is indeed working now and stops the rendering. The problem is that when the rendering restarts, the unsynced updates of the two arrays are still... well, unsynced, thus not solving the original problem. So basically the events are the following:

Now, I have no idea how to fix this problem. I will definitely look into it on my own, but maybe you have an idea on how to proceed.

Thanks in advance.

EDIT: At this point, I feel like the right way is to reassign the whole BufferGeometry, but I am not sure if the memory leaks that started my direct modifications of the arrays are fixed yet.

vidartf commented 4 years ago

Some notes:

sirbardo commented 4 years ago

@vidartf Yes, the arrays do change shape.

I made a notebook that shows the problem with pure pythreejs and just our data structure (which just reads a mesh and outputs numpy arrays relative to the geometry, no threading there, and the arrays "work" because you can see the geometry that is being output). Just clone the repo and run the notebook and you will see the problem (geometry updating before the wireframe).

https://github.com/cg3hci/py3DViewer/blob/devel/minimal_problem.ipynb

I don't have your version with the renderer hold installed right now so I made it without, but you can try with your local version and the same notebook, even with the "with renderer.hold():" it still exhibits the same problem.

I hope this code is clearer. Let me know. I'll continue testing other ways to fix this, meanwhile.

EDIT: Something to note is that I realized that my "linesegments", for stupidity on my part, is extremely inefficient (edges are sometimes repeated more than 8 times), therefore it is heavy enough to cause the unsync. I feel like once I will "slim down" the wireframe threejs could probably update both arrays fast enough that the problem is solved in sub-frame time, and therefore stops being noticeable. I will update with this once I optimize my wireframe.

sirbardo commented 4 years ago

@vidartf your comment made me want to be a little more strict with the buffer arrays to try to find a way to fix my problem myself, and therefore I started using indexed arrays of fixed preallocated larger size together with drawrange, to avoid changing the array size at runtime. This now made me run in another (reproducible, and easily so this time) problem. I opened another issue about this: #294

vidartf commented 4 years ago

Just clone the repo and run the notebook

I was more hoping for a minimal example (copy paste 20 lines of code). I don't really have enough time to debug this at depth if starting that far away.

sirbardo commented 4 years ago

Just clone the repo and run the notebook

I was more hoping for a minimal example (copy paste 20 lines of code). I don't really have enough time to debug this at depth if starting that far away.

Alright, I’ll try to make a pythreejs-only reproducible example, the main problem is that a large geometry is required for the problem to be noticeable, but I’ll find a way around it ASAP. In the meanwhile, the other problem with indexed arrays on the other issue is pythreejs-only and about 20 loc.