WIP: Partition model and interaction#75
Conversation
| self._views: list[model.View] = [] | ||
| for view in canvas.views: | ||
| self._snx_add_view(view) | ||
| self._filter = app().install_event_filter(self._snx_get_native(), canvas.handle) |
There was a problem hiding this comment.
All of this stuff should just go into scenex.interaction - would be a very easy change
| return # No change needed | ||
| # Compute the quaternion needed to rotate from the current up direction to | ||
| # the desired up direction |
There was a problem hiding this comment.
There's tons of deleted comments describing math-heavy functions that should be recovered
|
|
||
| # Step 2: Adjust the transform matrix to maintain the position | ||
| # under the cursor. The math is largely borrowed from | ||
| # https://github.com/pygfx/pygfx/blob/520af2d5bb2038ec309ef645e4a60d502f00d181/pygfx/controllers/_panzoom.py#L164 |
There was a problem hiding this comment.
More examples of comments that did not make it through Claude
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #75 +/- ##
==========================================
- Coverage 88.09% 79.48% -8.61%
==========================================
Files 64 68 +4
Lines 3174 3275 +101
==========================================
- Hits 2796 2603 -193
- Misses 378 672 +294 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| from scenex.model._view import View | ||
|
|
||
|
|
||
| class CameraController(EventedBase): |
There was a problem hiding this comment.
Should consider whether this should actually be an evented - but that has little to do with this PR.
From what I remember, I figured there might be utility in tracking Orbit.center - but there's nothing with PanZoom that would need to be tracked.
tlambert03
left a comment
tlambert03
left a comment
There was a problem hiding this comment.
broadly speaking I think I like general concept of moving the controller off the camera.
as an API, I don't particularly like:
canvas = snx.show(view)
ci = snx.CanvasInteractor(canvas)
ci.set_controller(view, snx.PanZoom())One of the main things I wanted from scenex (and for microvis before it) was a clean mental model, and one large hierarchical declarative model. And ideally, I don't want the common use case to have to import a bunch of classes like snx.CanvasInteractor and snx.PanZoom... that feels very much like just a repeat of what vispy and pygfx already do (making scenex feel less like a declarative scene-graph grammar, and more like just a wrapper around other scene graphs). I wanted the API of scenex to be self-discoverable from the models themselves. For example: rather than saying "ok, what does scenex export at the top level and what do each of these things do..." I would prefer the API-learning experience to be "ok, what fields does this Canvas object have, and what values/types do they each accept". The latter, reveals complexity gradually, on-demand, as the user digs deeper. The former presents a large API surface and expects the user to read more documentation to figure out which ones they really care about first.
I can imagine the Canvas model getting an interactor field that just has sane defaults, and has settable fields:
canvas.interactor.controller = 'panzoom'and, as always, it should all still serialize in a single model object, so that someone could, for example, spin up another canvas that had exactly the same controller properties with something like canvas = Canvas.model_validate(some_other_serialized_canvas).
Also, inasmuch as some models have graphics-backend adapters and others don't (which is fine and makes sense), I would not want the high level user to have to be aware of that distinction. It should just be an implementation detail. They shouldn't need to know when a field (like canvas.interactor) is one type of thing and, say, line.color is another type
2 participants
As scenex has grown, the
scenex.modelmodule has grown to encompass two different types of models:Canvas-Views and the scene graph they displayCameraControllers,ResizePolicyobjects, and the like.@tlambert03 has noted this (explicitly or not) a couple times, and I've also been finding myself wanting a little more of a separation between "here's what's on the canvas" and "here's how it moves".
With this PR, I asked Claude to try and separate out the two types of models. Type 1 models still live in
scenex.model, while type 2 models live inscenex.interaction. Now, the major modules (are / should be):scenex.model: What should be on the canvas. No dependencies on other scenex modulesscenex.app: Canvas widget/event loop utilities (for qt, jupyter, anywidget, wx, e.tc). No dependencies on other scenex modulesscenex.adaptors: Pathways for visualizing the canvas (i.e.pygfx/vispy). Dependencies onscenex.model, light dependencies onscenex.appscenex.interaction: Real-time model interaction. Dependencies onscenex.model,scenex.appThis changeset is in early stages and is nowhere near ready for merge. I've hardly looked at the internal implementation and I already have a good number of things I want to change. With that being said, all the tests pass with minimal modifications and the general paradigm seems to align with my requirements, so I wanted to push it early to allow for discussion before I invest more time.
@tlambert03 if you're willing, take a look at the examples and/or the changes to
scenex.modelwhen you get a chance. Do you like this separation of concerns better? Do you want to see where further changes would go here? Would love to hear your thoughts