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.

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

Sync time axis for multiple TimeSeriesView #7931

Open
jviereck opened this issue Oct 29, 2024 · 49 comments
Open

Sync time axis for multiple TimeSeriesView #7931

jviereck opened this issue Oct 29, 2024 · 49 comments
Labels
🟦 blueprint The data that defines our UI enhancement New feature or request 📺 re_viewer affects re_viewer itself

Comments

@jviereck
Copy link
Contributor

In my setup I am displaying data in two TimeSeriesView. The TimeSeriesView are positioned in a vertical viewport above each other. To compare the data from multiple axis, I have to manually position the x-axis (time in this case) to align well. It would be great if there would be a way to keep the two time-/x-axis in sync between the two views.

I was wondering if there could be a flag on the vertical viewport to sync the x-axis for contained TimeSeriesViews.

If someone could provide me with some pointers on how to implement this feature, I am more than happy to work on a pull request.

@jviereck jviereck added enhancement New feature or request 👀 needs triage This issue needs to be triaged by the Rerun team labels Oct 29, 2024
@Wumpf Wumpf added 📺 re_viewer affects re_viewer itself 🟦 blueprint The data that defines our UI and removed 👀 needs triage This issue needs to be triaged by the Rerun team labels Oct 29, 2024
@Wumpf
Copy link
Member

Wumpf commented Oct 29, 2024

If someone could provide me with some pointers on how to implement this feature, I am more than happy to work on a pull request.

I think this boils down to being able to connect blueprint properties of views with each other, so this might quickly turn into a more general infrastructure task.
Essentially this property should be automatically sourced from a different view or by implementing an inheritance scheme for containers (that would be closer to your idea of putting it on a container).

A more short-term solution that's worth experimenting with would be to add an optional view id to that property archetype specifically.

I'm surprised we didn't have an issue for this already (couldn't fine any at least)

@teh-cmc
Copy link
Member

teh-cmc commented Oct 29, 2024

In my setup I am displaying data in two TimeSeriesView. The TimeSeriesView are positioned in a vertical viewport above each other. To compare the data from multiple axis, I have to manually position the x-axis (time in this case) to align well. It would be great if there would be a way to keep the two time-/x-axis in sync between the two views.

Is there anything in particular in your use case that is preventing you from displaying the two timeseries in the same view?

@jviereck
Copy link
Contributor Author

Is there anything in particular in your use case that is preventing you from displaying the two timeseries in the same view?

The y-scale of the data is very different. This makes it hard to show the data in the same TimerSeriesView. For this task, it would be possible to have to Y-axis plotted in one TimeSeriesView. However, this works only for two y-axis and doesn't scale for more Y-axis or separating out the data into multiple TimeSeriesViews (e.g. one TimeSeriesView for forces, one for centroidal position, one for torques of a robot etc).

@jviereck
Copy link
Contributor Author

jviereck commented Oct 30, 2024

I think this boils down to being able to connect blueprint properties of views with each other, so this might quickly turn into a more general infrastructure task.

I wonder if we can have one global time-series view x-axis property that the individual time series can use or not. This would make the implementation easier as there is only one global property to sync and this doesn't need to be implemented generic enough for all properties. As this is a toggle-on / toggle-off property on the TimeSeriesView, this becomes a boolean flag, which might be easy to add to the TimeSeriesView.

This leaves me with two questions about the rerun infra and implementation (for now):

  • Is there a way to pass down a shared state through all the components to all the views?
  • Is there a kind of event system to listen to zoom / position changes in the TimeSeriesViews and then update the global x-axis range?

@jviereck
Copy link
Contributor Author

jviereck commented Nov 1, 2024

@Wumpf @teh-cmc Do you have a pointer for me how to update the global state to keep track from a shared x-axis range from the TimeSeriesViews?

@Wumpf
Copy link
Member

Wumpf commented Nov 4, 2024

Is there a way to pass down a shared state through all the components to all the views?

no, not yet. One part of our vision in this area is that containers can have arbitrary view properties set that then propagate down the tree and are used whenever a view doesn't set anything. I.e. the view property accessors/queries would be aware of an inheritance tree.
That idea doesn't account for a view changing the property of its parents rather than writing to its own, but yeah some kind of toggle that would make it propagate upward rather than set things locally might be a good way to go about this! 🤔

Is there a kind of event system to listen to zoom / position changes in the TimeSeriesViews and then update the global x-axis range?

Generally, all views operate in isolation and are very free in how interactions are implemented. In the case of the time series view this is for the most part just egui plot. There's already properties that the interaction writes out to the blueprint store e.g. here

scalar_axis.save_blueprint_component(ctx, &new_y_range);

@jviereck
Copy link
Contributor Author

jviereck commented Nov 4, 2024

Thanks for your comments @Wumpf .

Over the weekend I looked at the code a bit. It seems like the egui_plot has a build in way to link axes of different plots together. See here the example from egui_plot: https://github.com/emilk/egui_plot/blob/main/demo/src/plot_demo.rs#L669

You can play with a demo of the linked axes here: https://emilk.github.io/egui_plot/ (click on the "Linked Axes" tab on the top).

If we use this linked axes feature, the TimeSeriesViews could have a new property like "SharedAxesNames" (which is a string). The TimerSeriesViews with the same SharedAxesNames will then be synced.

Do you think this is worth exploring further?

@Wumpf
Copy link
Member

Wumpf commented Nov 4, 2024

it could be that we also need to use something like this to avoid frame delays, but generally I don't think that's a good direction to take as that would completely sidestep the state we have in the blueprint and thus what is controlled from api, shown in the ui and stored on disk

@jviereck
Copy link
Contributor Author

jviereck commented Nov 4, 2024

Makes sense to want to keep things stored in the blueprint.

If we would store a hash-map with SharedAxesNames -> range in the blueprint with a similar call to scalar_axis.save_blueprint_component(ctx, &new_y_range); , would that be the way to go?

@Wumpf
Copy link
Member

Wumpf commented Nov 4, 2024

yeah I guess something like that could work out for starters :)
as implied I really don't want to special case this too much, but if we don't want to wait for a grand unified system here, this gotta start with something more rough-edged 😄
ui for setting up these connections would be challenging for that (either way), but I'd suggest making the axis linking only accessible from (python) code for now and have it break automatically if someone edits the axis from the view selection menu 🤔

@Wumpf
Copy link
Member

Wumpf commented Nov 4, 2024

Another thing I didn't think of before is that you'll need some "new special place" to store and write that map though. Right now the structure of the blueprint store is fairly rigid and very very undocumented. But in a nutshell there's a definition of a container hierarchy with the leaf level, the views, having a bit or leeway to store arbitrary properties in sub-entities.
So for this to work you'd ofc need to find a place in that hierarchy for those new "global properties"

@jviereck
Copy link
Contributor Author

jviereck commented Nov 6, 2024

I managed to implement a shared-x-axis. At the moment all TimeSeriesViews use the same shared x-axis. The shared x-axis is written to the blueprint. The code is quite hacked and I am not sure it will work well when the data is streaming in and the x-axis gets updated to the last x seconds.

@Wumpf : Would you mind taking a link on the current changes and tell me what you think? I was wondering if there should be a SharedAxesNames on the TimeSeriesSpaceViewState which determines which TimeSeriesViews to link. Is there a way to set this string as a property in the property panel on the right side (like in a text box)? Otherwise, you mentioned this feature could be programmatically only for now. Let me know what you think should be implemented first.

My current code is here: https://github.com/jviereck/rerun/tree/issue-7931-sync-x-axis

Video of the changes:

Screen.Recording.2024-11-05.at.22.32.18-1080-h264.mov

@Wumpf
Copy link
Member

Wumpf commented Nov 6, 2024

Would you mind taking a link on the current changes and tell me what you think?

Hum, well yeah sure it's a hack as you say 😉. From what I can tell it also doesn't interact well with any of the other properties that the view already tries to write, so not a friendly one to what's there either. (edit: not sure sure about that actually, see notes below on how X axis isn't properly implemented in the first place) Haven't thought through the entire chain of interaction with the egui-plot; I found this quite taxing in the past since egui plot tries to keep its own state and we both infer ours from how it changes and try to apply it at the same time :/

I was wondering if there should be a SharedAxesNames on the TimeSeriesSpaceViewState which determines which TimeSeriesViews to link.

Yeah I think the linking direction could make a lot of sense and aligns well with the vague plane of entity links we wanted to do in the future! That also sidesteps the issue we talked about previously here on where and how to store the shared axis.
To that end, what you could experiment with is to add a sort of link component to the view properties. Nothing is ever easy, so here's also a bunch of things to solve there:
The properties of the view are defined here https://github.com/rerun-io/rerun/blob/main/crates/store/re_types/definitions/rerun/blueprint/views/time_series.fbs. Something I wasn't fully aware until just now is that we don't actually store the x range yet there which is also why it doesn't show up in the selection ui. Yes there is VisibleTimeRange, but that one is about what is queried. I had proposal to have that one directly linked to the plot navigation, but within the team we decided that what is on screen should be different from what is queried (@teh-cmc am I doing this justice? ;)).
So...

  • first we need a new property archetype that defines what's actually visible on screen left/right
    • this needs to be synced up such that it is shown in the ui and can be manipulated directly. Essentially the exact same thing as with ScalarAxis
  • then, this property needs a way to source its element from a different location

... well as I said, this won't be easy since so much infrastructure is missing to do this kind of thing 😅.
But this conversation convinced me that the initial comment on linking entities/properties is the right way (even if such a connection is only implemented for a single type) and much more viable than a inheritance based one :)

@Wumpf
Copy link
Member

Wumpf commented Nov 6, 2024

Hmm actually I guess for what you want you want to specifically link the VisibleTimeRange property. So maybe it is "just" about it having a UUID or even better an EntityPath to link someone elses timerange? 💡

@Wumpf
Copy link
Member

Wumpf commented Nov 6, 2024

first we need a new property archetype that defines what's actually visible on screen left/right

This could look something like this:

table TimeAxis (
    "attr.rerun.scope": "blueprint",
    "attr.rust.derive": "Default"
) {
    /// The range of the axis.
    ///
    /// If unset, the range well be automatically determined by the visible time range of the view.
    range: rerun.components.Range1D ("attr.rerun.component_optional", nullable, order: 2100);

    /// If enabled, the time axis range will remain locked to the specified range when zooming.
    zoom_lock: rerun.blueprint.components.LockRangeDuringZoom ("attr.rerun.component_optional", nullable, order: 2200);

    // ⬅️ Add a hack for linking to another view here.
}

@jviereck
Copy link
Contributor Author

jviereck commented Nov 6, 2024

Thanks for your comments @Wumpf .

Hmm actually I guess for what you want you want to specifically link the VisibleTimeRange property. So maybe it is "just" about it having a UUID or even better an EntityPath to link someone elses timerange? 💡

What I did in the implementation so far is creating a UUID from a string like this:

        let shared_id = SpaceViewId::hashed_from_str("TimeSeriesShared");

I was thinking about the SharedAxesNames property to be a string and then this string is hashed to get a UUID. So the only thing we would need to add on the property panel to the right would be a text box for the shared-axes-name.

Would that work?

@Wumpf
Copy link
Member

Wumpf commented Nov 6, 2024

it could work with some more hacks and custom ui. But really I'd like that to be a configurable uuid of an existing view and not a made-up one. I'm not entirely sure about the repercussions of breaking the blueprint entity hierarchy in such a way, so please understand that I'd be very hesitant to land it like that.

@jviereck
Copy link
Contributor Author

jviereck commented Nov 6, 2024

Thanks @Wumpf for your last reply. That makese sense. I will explore adding a linkage in the table TimeAxis.

@Wumpf
Copy link
Member

Wumpf commented Nov 6, 2024

Thanks for being so understanding and patient on this, especially given that I clearly don't quite know yet exactly how to solve this myself apart from some vague future plans 😅
We still have a lot of work in front of us to get the internals better documented 🤔 . As you can tell a big part of that is that the dust hasn't quite settled yet and there's many decisions we'd like to iterate on.

On a related note: One thing that's very useful for debugging & understanding the blueprint hierarchy is the blueprint timeline visualization which is an option in Debug builds only right now (we want to make it an exposed thing, but it doesn't work super well yet and it looks kinda bad)
image

@jviereck
Copy link
Contributor Author

jviereck commented Nov 7, 2024

So my current idea is to add a new sync_with field on a table with optinal type re_viewer_context.SpaceViewId. If this field is set, the TimeSeriesView looks up the x-axis from this other TimeSeriesView and thereby synces them. I wonder how this field sync_with would be populated. Assuming this is set during the blueprint definition from the SDK, it looks like the python SDK definition for the TimeSeriesView is here:

Looking at the generated code in here: https://github.com/rerun-io/rerun/blob/main/rerun_py/rerun_sdk/rerun/blueprint/views/time_series_view.py#L143

It looks like the fields on the table TimeSeriesView can be set from Python. This requires the value to be of blueprint_archetypes though. I wonder if instead of putting the sync_with field on the table TimeAxis as proposed before by @Wumpf , this new field should go on the table TimeSeriesView instead?

Assuming the field sync_with goes on table TimeSeriesView, it looks like there is no archetype for re_viewer_context.SpaceViewId yet.

Does it therefore make senes to implement a new re_viewer_context.SpaceViewId archetype?

@jviereck
Copy link
Contributor Author

jviereck commented Nov 7, 2024

Other idea might be to add a string name to table TimeSeriesView and then have sync_with be a string as well. When drawing the TimeSeriesView, there could be a lookup-by-name function that returns the SpaceViewId of the TimeSeriesView with a given name.

@Wumpf
Copy link
Member

Wumpf commented Nov 8, 2024

I pondered this a lil bit more and synced with @jleibs to coordinate what solution we'd like to have medium term. Leading to the writeup of this related issue documenting the issues with the time axis in the absence of syncing

With that context established, back to this issue and let's say we want to skip #8050 and jump ahead as quickly as possible :). Jumping a bit for didactic reasons:

Does it therefore make senes to implement a new re_viewer_context.SpaceViewId archetype?

Sort of! I think what we want is a new LinkedSpaceViewId component which then lives on the TimeAxis property-archetype that is defined in #8050. If, for expediency, we want to leave out all the other fields that were proposed there, we can just (somewhat similar to your first prototype) store a hacky globally time range but do so for every view.
Each view that has the LinkedSpaceViewId set can then grab that time range and apply it!

Other idea might be to add a string name to table TimeSeriesView

That's not quite how the structure on the views works: The idea is is that each view has a series of property archetypes, each containing a bunch of components defining the property.
In the blueprint store each of these archetype actually goes into a sub-entity of the entity-path that represents the view itself, which allows overlapping components between those different property archetypes (i.e. those archetypes are there to establish a sort of context)

It looks like the fields on the table TimeSeriesView can be set from Python.

Yes. Let me clarify a bit further: (almost) everything that is defined in those fbs files is available from all SDK languages (it's just that some important scaffolding is missing so far to have blueprint be accessible from C++ and Rust). And vice versa every built-in type that Rerun stores in blueprint or the store comes from these.

Hope that makes sense!

@jviereck
Copy link
Contributor Author

jviereck commented Nov 9, 2024

Thanks for the write up. I am a bit confused what to do.

can just (somewhat similar to your first prototype) store a hacky globally time range but do so for every view.

What do you mean by "global"? Can you maybe draw an outline where in the blueprint debug pannel these entries would live?

Each view that has the LinkedSpaceViewId set can then grab that time range and apply it!

When constructing the TimeAxis in python, how would I create the LinkedSpaceViewId and then reference to it from multiple TimeSeriesViews?

Also, would the TimeSeriesViews just lookup the LinkedSpaceViewId and it would point to the same object from multiple views? Is the field treated as a pointer that can be shared?

@Wumpf
Copy link
Member

Wumpf commented Nov 11, 2024

What do you mean by "global"? Can you maybe draw an outline where in the blueprint debug pannel these entries would live?

Global was a bit of a misnomer: The thing you did in your prototype I would have called global because it isn't tied to the entity of a specific view (whose path is derived from the view's id). But it would be ofc better to put it in a subpath of a view, so it's not really global but also doesn't quite follow the current system since there wouldn't be a formal definition (via fbs) where that data is (unless all of #8050 is implemented :)).

When constructing the TimeAxis in python, how would I create the LinkedSpaceViewId and then reference to it from multiple TimeSeriesViews?

For decent ergonomics this requires some work on the Python API, but for a less fluid api this should be already possible I believe: the ids are created on the python side in SpaceView.__init__. So if one adds the LinkedSpaceViewId component after creating the "parent" view, the id should be available.

Also, would the TimeSeriesViews just lookup the LinkedSpaceViewId and it would point to the same object from multiple views? Is the field treated as a pointer that can be shared?

yeah my idea would be to treat LinkedSpaceViewId like a pointer of sorts: you can create an entity path pointing "inside" a different view with it and read out its data, thus creating the link.

@jviereck
Copy link
Contributor Author

Thanks for the explanations @Wumpf.

But it would be ofc better to put it in a subpath of a view, so it's not really global but also doesn't quite follow the current system since there wouldn't be a formal definition (via fbs) where that data is (unless all of #8050 is implemented :)).

Would this data still be saved in a blueprint? If so, how can data be loaded and stored without a fbs?

I am worried side stepping the fbs and other infrastructure will make solving this issue quite a hack. I am therefore leaning towards implementing the fbs from #8050 for this feature.

What do you think?

@Wumpf
Copy link
Member

Wumpf commented Nov 11, 2024

Would this data still be saved in a blueprint? If so, how can data be loaded and stored without a fbs?

yes. Those fbs files are strictly just for the codegen that generates types (and their serialization!) for all sdk languages. But that doesn't prevent just making up stuff that isn't defined in those. Otherwise custom component/data types wouldn't work either.

doing #8050 first (and separately) would definitely be preferable, yes :). I was just looking for ways to cutting corners to speed this up 🤷

@jviereck
Copy link
Contributor Author

jviereck commented Dec 2, 2024

Let's assume #8050 is implemented, what would be the way to link multiple TimeSeriesViews together (where linking together means to share the view range and query data range)?

@jviereck
Copy link
Contributor Author

I am not planning to work on this.

@Wumpf , do you have a timeline by when this will be implemented?

@Wumpf
Copy link
Member

Wumpf commented Dec 19, 2024

I was really hoping to get to that sooner, but other things keep cropping up, so no timeline unfortunately

@jviereck
Copy link
Contributor Author

jviereck commented Jan 9, 2025

FYI, I started working on an alternative approach: Instead of syncing multiple TimeSeriesViews, my version of the TimeSeriesView is capable to display multiple timeseries above each other. By adding a scrolling container, this makes it possible to scroll through a list of timeseries plots easily.

For this to work, the interaction with the plot is very different compared to the current one. There are also some other features I am adding that makes determining what to plot easier when you have data with a lot of dimensions.

Would having such a TimeSeriesView make sense to be integrated with rerun directly? I am willing to contribute my code and would be much easier for me if it could go into the main branch.

@Wumpf
Copy link
Member

Wumpf commented Jan 10, 2025

definitely curious about this! I believe we considered containers with larger virtual area in the past, but never designed it out
cc: @abey79, @gavrelina

@abey79
Copy link
Member

abey79 commented Jan 10, 2025

Yes, I'm a big proponent of going the "subplots-within-a-single-view" way instead of trying to sync axes across separate, possibly arbitrarily laid out views. We've discussed this at a workshop during our last offsite and I made a proposal to that effect, but this has yet to be fleshed out into a concrete design (or even an issue).

Digging into the specifics since you mention a scrolling container: I would much like to explore alternatives to that. Plots are scrollable, and, in my experience, having scrollable things inside of a scrollable thing makes for a poor UX.

Off the bat, my idea would be to always fit the full subplot grid to the available space, and maybe to collapse it to a single plot with a mini-map if space becomes tight. (Surely @gavrelina will have much better ideas.) That would obviously only work for a limited number of plots (say, less than 10ish). Do you have a use case that requires many more plots? If so, could you expend on it?

@jviereck
Copy link
Contributor Author

Thank you for your feedback. I finished a first prototype. You can see a short demo in this video: https://youtu.be/5yKvZmUyPVo

You can find the code here: https://github.com/jviereck/egui_plot/tree/timeseries_split_view

Just call cargo run and the prototype should come up.

@jviereck
Copy link
Contributor Author

jviereck commented Jan 11, 2025

Thanks for your input @Wumpf and @abey79 .

The proof of concept I have implemented is based on a previous timeseries plotter I wrote during my PhD. The code is here: https://github.com/KyberLabsAI/mim_data_utils/blob/main/index2.html.

When working on robots (think humanoid robot), there are hundreds of data points coming at every timestep (where a timestep is in the range of 1000 Hz to 30,000 Hz). Doing the current approach from rerun with manually toggeling on and off individual data lines doesn't scale well. As the data is oven packed as on vector (e.g. joint positions as q), the path of the data looks like /data/q[0], /data/q[1], .../data/q[62], so doing a filter on the path alone is not helpful.

What I found helpful is to specify which data to display in which plot using a text input field. When the plots get too complicated, I used a script to generate the plot definition and then inserted it to the plotter (e.g. show all the forces, joint positions and joint velocities for the front-left leg of a quadruped as well as the IMU angular velocities in local and world frames).

The string to specify the plot layouts support slicing in my implementation: Writing q[0:8] plots the first 9 values from the array. To create multiple plots, you use a pipe symbol: q[:3] | dq[:3] where q are the joint positions and dq the joint velocities of a robot (in this case, this would show the front-left leg data from our 12 degree of freedom quadruped).

Digging into the specifics since you mention a scrolling container: I would much like to explore alternatives to that. Plots are scrollable, and, in my experience, having scrollable things inside of a scrollable thing makes for a poor UX.

In my concept, the y-axis is not scrollable. Instead, it auto-scales based on the current displayed y-data range. This works remarkable well when working on my real live applications so far.

Off the bat, my idea would be to always fit the full subplot grid to the available space, and maybe to collapse it to a single plot with a mini-map if space becomes tight.

I am using a fixed size of 300 px height at the moment. If there is enough space, the plots could be higher but if the plots become less than e.g. 300 px in height, I would start scrolling.

Do you have a use case that requires many more plots? If so, could you expend on it?

See the example above for plotting joint positions, joint velocities and IMU data. As each of these quantities has different data ranges, it is hard to plot them together. Still, you want to see correlations between the data (e.g. when did a joint stop moving and did the IMU pick it up?) by having the plot data aligned along the x axis with each other.

@jviereck
Copy link
Contributor Author

Hi, is there any feedback on my design proposal / proof-of-concept?

If this looks fine with you, I would start an implementation in rerun.

@abey79
Copy link
Member

abey79 commented Jan 20, 2025

Awesome work with the prototype! Here is some feedback.

Layout

In my concept, the y-axis is not scrollable. Instead, it auto-scales based on the current displayed y-data range. This works remarkable well when working on my real live applications so far.

Although his is a very useful feature, I think it is too opinionated to be enforced. So IMO, the scrollable-thing-in-a-scrollable-thing issue remains.

I am using a fixed size of 300 px height at the moment.

This is also a bit too opinionated. Of course, you might offer a setting of that that, but that would become an alternative sizing mechanism to the existing one (aka the current view/container tile system of the viewport), something I'd rather avoid.

Do you have a use case that requires many more plots? If so, could you expend on it?

See the example above for plotting joint positions, joint velocities and IMU data. As each of these quantities has different data ranges, it is hard to plot them together. Still, you want to see correlations between the data (e.g. when did a joint stop moving and did the IMU pick it up?) by having the plot data aligned along the x axis with each other.

For sure, stacked plots are useful. My question was about huge stacks, eg. more than could reasonably fit on a single screen without scrolling.

As is clear from the above, I am in general reluctant to introduce within views layout features which exist (or should exist) at the viewport level. In that sense, the mere idea of stacked plots within a single plot view is arguably "wrong" on the conceptual level. Since it is hard to tweak the current viewport paradigm to achieve axes alignment (we tried), the stacked plot view we're discussing is IMO the right way forward—despite its possible conceptual flaws. We should however strive to be minimalistic in the layout feature we introduce.

Based on that framework, I believe the plot view should behave as follows:

  • Configurable number of rows and column.
  • View entities can be assigned to any (or many?) of the subplot.
  • X axes are always synchronised across columns.
  • Y axes are always synchronised across rows.
  • Each sub-plot has the same size, such that they fit the current view size.

UI

Your DSL proposal is interesting. I see this being very useful to accelerate the building of a prototype. Because it "competes" (again 😀) with the existing entity path filter DSL (which will evolve in the future), I don't think we could keep it for the final feature. But I'm confident that with the help of @gavrelina we can figure out and implement a suitable configuration UI.

@jviereck
Copy link
Contributor Author

Hi @abey79 ,

Thank you for you're feedback. This makes sense.

with the existing entity path filter DSL (which will evolve in the future), I don't think we could keep it for the final feature. But I'm confident that with the help of @gavrelina we can figure out and implement a suitable configuration UI.

I like the idea to extend the existing path filter DSL (I have not thought about it much).

I wonder if something like this could work

**

plot 0:
**trig[1:3]

plot 1:
**pow

Where you can specify first a general match and then a per-plot match for data to display. Ideally, the match would understand the slicing syntax (like 1:3). I like the idea to specify the data for all the plots in a single input field (vs a input field per plot). This allows generating a layout string before and paste the entire layout.

What do you think?

@abey79
Copy link
Member

abey79 commented Jan 20, 2025

What do you think?

It's an interesting idea, but that particular DSL is at the crossroads of many other things (both that exist and are to come), so it's rather tricky to design. I suggest not going in that territory for the present issue.

My current idea for the configuration UI would be to have a sort of minimap in the selection panel, where each subplot is represented by a grey box. The number and arrangement of these box would depend on the chosen number of rows/column. By default, all entities would land in the top-left box, and could be dispatched to other boxes by drag-and-drop.

@jviereck
Copy link
Contributor Author

jviereck commented Jan 20, 2025 via email

@abey79
Copy link
Member

abey79 commented Jan 20, 2025

I am worried that dran-and-drop will be very time consuming to setup the
subplots. Especially if every dimension of a vector is a independent drag
and drop. I would prefer a text bases selector pattern.

Making this UI efficient is a challenge for @gavrelina. However, we are most likely not rolling out a super-advanced UI for "extreme" case at the expense of ease-of-use and learning curve for simpler (and likely more common) use case.

IMO, the correct way to setup complex sub-plots involving lots of entities is to use the blueprint API (which, by the way, will have to be extended to nicely cover the sub-plot case).

@jviereck
Copy link
Contributor Author

Making this UI efficient is a challenge for @gavrelina.

Okay, I leave the design for the property panel to @gavrelina then and focus on setting things up using the blueprint API only.

IMO, the correct way to setup complex sub-plots involving lots of entities is to use the blueprint API

Makes sense. I can design some blueprint API and share it here.

@jviereck
Copy link
Contributor Author

As for the API, the current API to specify a TimeSeriesView looks like this:

    rr.send_blueprint(
        rrb.Grid(
            contents=[
                rrb.TimeSeriesView(
                    origin='data/',
                    time_ranges=[
                        rrb.VisibleTimeRange(
                            "time",
                            start=rrb.TimeRangeBoundary.cursor_relative(seconds=-window_size),
                            end=rrb.TimeRangeBoundary.cursor_relative(),
                        )
                    ],
                    plot_legend=rrb.PlotLegend(visible=True),
                )
            ]
        ),
    )

To make it work for subplots, I was thinking to add a rows and columns entry on TimeSeriesView To specify the content per subplot, a new sub_plots entry could be added. The sub_plots entry takes an array of SubPlotData. This looks like this:

rrb.TimeSeriesView(
  origin='data/',
  ... # Other current fields.
  row=3,  # Optional - if not provided default value is 1.
  columns=1  # Optional - if not provided default value is 1.
  sub_plots=[
    rrb.SubPlotData(row: 1, column: 1, filter: 'data/trig'),
    rrb.SubPlotData(row: 2, column: 1, filter: 'data/pow[1]'),
    rrb.SubPlotData(row: 3, column: 1, filter: 'data/trig[1]'),
    rrb.SubPlotData(row: 3, column: 1, filter: 'data/pow[1]')
  ]
)

The filter specifies which part of the TimeSeries available after applying the TimeSeriesView's outer origin. Note that you can have multiple SubPlotData for a subplot (e.g. the last two entries in the sub_plots array reference the row=3, column=1 subplot. This allows to specify the data that goes into one subplot more precisely / with multiple filters.

What do you think?

@tblaha
Copy link

tblaha commented Jan 24, 2025

Hey all, great work! I'm really looking forward to this feature!

As an intermediate solution that potentially require less changes, would it be possible to use the range-selection feature in the time-line to update all TimeSeriesView's x-axes? Most, if not all of my usecases would be satisfied with that.

Image

@jviereck
Copy link
Contributor Author

I took another take at the scalar_axis.save_blueprint_component(ctx, &new_y_range); idea I mentioned before. I know this might be a hacky solution, but I got something decent to work. See video enclosed (I reduced the resolution to not upload a big video to GitHub). The changes are rather small and can be found in 1.

I feel this is a much easier solution that trying to implement multi-plots-in-one-container approach.

Let me know what you think.

Screen.Recording.2025-01-25.at.11.10.54.mov

@jviereck
Copy link
Contributor Author

When trying my hacked version, I noticed that the x-axis don't align. Turns out that the space for the plot depends on the size of the y-labels. See enclosed. I guess the y-label space should be synced between plots as well.

Image

@abey79
Copy link
Member

abey79 commented Jan 29, 2025

Yes, these are a few of the many reasons I believe the "sub-plot" approach is easier than the "inter-view-synchronisation".

Your blueprint API proposal look pretty good indeed. Most of the design complexity here lies in the per-sub-plot filter. It should probably have the same syntax as the view's entity path filter. But does it replace it? Or is it applied on top of the view filter? Neither feel particularly well integrated in the larger picture of how views generally work. I wonder what could be the alternatives.  @Wumpf do you have a take on this specific point?

@jviereck
Copy link
Contributor Author

@abey79 just checking - did you see my latest comment about progress on inter-view-synchronization? I ised this way to wync plots yesterday for some real world applications and it worked fine.

@abey79
Copy link
Member

abey79 commented Jan 29, 2025

just checking - did you see my latest comment about progress on inter-view-synchronization? I ised this way to wync plots yesterday for some real world applications and it worked fine.

I saw that, and yeah it's pretty nice, but I'm still unconvinced of the UX of this approach (what if views are moved around? how do you deal with multiple groups of views which should by independently synchronised?) and its architectural implications (as per your "//HACK" code comment 🙃). In other word, there is a great deal of work left to make that shippable.

@jviereck
Copy link
Contributor Author

Thank you for your comments @abey79 !

but I'm still unconvinced of the UX of this approach (what if views are moved around?

The moved plots keep syncing with respect to each other. That's what I would expect to happen. Do you have other expectations?

how do you deal with multiple groups of views which should by independently synchronised?)

My idea is to introduce a new field on rrb.TimeSeriesView - something like syncXAxisName. All TimeSeriesViews with the same syncName are synced together. This allows a flexible setup.

In other word, there is a great deal of work left to make that shippable.

Assuming the hack part is okay, I feel there is not much left to do.

I know the hack-solution doesn't look great, but it's working. I am worried that going with the "sub-plot" approach will be cleaner to implement but it will be much harder to figure out the UX: How do you assign the data to each subplot? How can you adjust the order of the subplots?

Going with the "sub-plot" approach creates a lot of new UX patterns that are different from the existing ones. The UX for the inter-view-synchronization approach follows very much how things are done in rerun otherwise (you see a tree of which data is plotted on the left panel etc, you can drag views around between containers etc).

So before all the UX for the "sub-plot" approach is figured out, I don't think this is the approach we should start to implement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🟦 blueprint The data that defines our UI enhancement New feature or request 📺 re_viewer affects re_viewer itself
Projects
None yet
Development

No branches or pull requests

5 participants