Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor re_viewer #1873

Closed
2 of 5 tasks
emilk opened this issue Apr 17, 2023 · 3 comments
Closed
2 of 5 tasks

Refactor re_viewer #1873

emilk opened this issue Apr 17, 2023 · 3 comments
Labels
📺 re_viewer affects re_viewer itself 🚜 refactor Change the code, not the functionality

Comments

@emilk
Copy link
Member

emilk commented Apr 17, 2023 •
edited by Wumpf
Loading

It is growing big and unwieldy. We should aim to break it up into more crates, and clean up some stuff.

Ideally re_viewer should only be UI code.

This needs some more planning before we execute, but some rough ideas:

  • Move blueprinty non-gui stuff to own crate (re_blueprint)
  • Break up app.rs
  • Refactor the different views (view_tensor, view_text, …), and maybe move them to their own crates
  • …
@emilk emilk added 📺 re_viewer affects re_viewer itself 🚜 refactor Change the code, not the functionality labels Apr 17, 2023
@Wumpf
Copy link
Member

Wumpf commented Apr 18, 2023 •
edited
Loading

Keep in mind while working on this

@Wumpf
Copy link
Member

Wumpf commented May 2, 2023

Started internal writeup on possible splits https://www.notion.so/rerunio/re_viewer-splitup-refactor-8f0c96406e5a4d5e9d030e74d7a6db34

This was referenced May 3, 2023
Merged
Merged
This was referenced May 4, 2023
Merged
Merged
Merged
Merged
This was referenced May 29, 2023
Closed
Merged
Wumpf added a commit that referenced this issue May 29, 2023
@Wumpf
[pure refactor] Separate viewport related files out to a new re_viewp…
00a7030 
…ort crate (#2251)

<!--
Open the PR up as a draft until you feel it is ready for a proper
review.

Do not make PR:s from your own `main` branch, as that makes it difficult
for reviewers to add their own fixes.

Add any improvements to the branch as new commits to make it easier for
reviewers to follow the progress. All commits will be squashed to a
single commit once the PR is merged into `main`.

Make sure you mention any issues that this PR closes in the description,
as well as any other related issues.

To get an auto-generated PR description you can put "copilot:summary" or
"copilot:walkthrough" anywhere.
-->

### What

First step towards:
* #2249 

and getting close to finish of:
* #1873 

This PR moves almost everything viewport related out to a separate crate
that is eclipses the `re_viewer` package in size now. In upcoming steps
space view definition will be separated out of this new crate in turn.
Also, blueprint things will likely need a new home as well similar to
`re_log_types`. But one thing after the other :)

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)

<!-- This line will get updated when the PR build summary job finishes.
-->
PR Build Summary: https://build.rerun.io/pr/2251
@Wumpf Wumpf mentioned this issue May 30, 2023
Merged
1 task
Wumpf added a commit that referenced this issue May 31, 2023
@Wumpf
Initial Space View trait & port of text space views to the new Space …
fe1869e 
…View trait system (#2281)

<!--
Open the PR up as a draft until you feel it is ready for a proper
review.

Do not make PR:s from your own `main` branch, as that makes it difficult
for reviewers to add their own fixes.

Add any improvements to the branch as new commits to make it easier for
reviewers to follow the progress. All commits will be squashed to a
single commit once the PR is merged into `main`.

Make sure you mention any issues that this PR closes in the description,
as well as any other related issues.

To get an auto-generated PR description you can put "copilot:summary" or
"copilot:walkthrough" anywhere.
-->

### What

Main pieces of:
* #2249 
* #1873 

Introduces a new framework for space view classes that will eventually
replace `ViewCategory` (right now the systems live side by side,
creating some oddities).
Ports text & text-box space views to this new system.

### Why

This paves the way for more structured space views, more streamlined
blueprint configuration and user defined space views.
In fact, this PR already enables user defined space views, but does not
yet expose a way to add them to the viewport (this can be done with some
small hacks to the space view adding code though and works very well!)

### Future work / discussion

There is still a lot of open question on the space view trait and it
will require changes as we move the other space views over to it.

Most notably, archetypes are defined by have no effect yet!
Overall, scene definition can be regarded as experimental at this point.
I chose to have a hard separation of `SceneElement` (the successor of
`ScenePart`) in the hope of providing a more powerful framework and an
easy hook for future parallelization. We'll need to see how this pans
out though!

Unlike planned, the definition of the space view class trait (as well as
implementation utilities) are not in a separate crate, but part of the
viewer context since global access to the space view type registry
proved very valuable.
We could still separate some parts of it out if desirable, but this
seems not very important at the moment.

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)

<!-- This line will get updated when the PR build summary job finishes.
-->
PR Build Summary: https://build.rerun.io/pr/2281
@Wumpf Wumpf removed their assignment Jun 9, 2023
@Wumpf
Copy link
Member

Wumpf commented Jun 9, 2023

landscape changed quite a bit by now. I'd call that done despite there being a bunch of things that could still be separated out (most prominently the Blueprint panel and the Selection panel) and App.rs could look better still

@Wumpf Wumpf closed this as completed Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
📺 re_viewer affects re_viewer itself 🚜 refactor Change the code, not the functionality
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@emilk @Wumpf