Skip to content

Conversation

@kratos2377
Copy link
Contributor

This is not the final change, this is just an approach i am taking to fix the following Issue: #11792

The approach is to basically keep a recording_context map based on storeId in viewer_context and set current selection if any recording switch happens. Currently testing it in my local but just wanted to get some feedback from you guys.
@abey79 @Wumpf @lucasmerlin

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Thanks for opening this pull request.

Because this is your first time contributing to this repository, make sure you've read our Contributor Guide and Code of Conduct.

@Wumpf
Copy link
Member

Wumpf commented Nov 21, 2025

Thanks for contributing!
I only had a super quick look: this change looks quite heavy and brittle right now, it's unclear to me why you need all this extra machinery. Isn't it enough to just make time_ctrl: RwLock<TimeControl>, a hashmap over stores/recordings or alternatively move it to a place were we already have per store data? That said I think I actually slightly disagree with the original ticket - it should still memorize this per application id and not per recording since everything within the same application id can be expected to be very similar.
Also note that since the ticket was created, quite a few properties actually moved into blueprint (or did that already happen before, not sure). Which means that things like timeline selection are already now associated per application id and it's a very deep change to change that. Afaik the only ephemeral/non-blueprint-stored things are now timeline cursor and selection range.
Since @abey79 created the ticket, let's see what he has to say when he is back and has a spare minute :)

@kratos2377
Copy link
Contributor Author

Hey @abey79 , would love to hear your thoughts on this as well. I kind of agree with @Wumpf here but would love to get some feedback from you as well before moving forward with the finalising the approach to handle this issue.

@abey79
Copy link
Member

abey79 commented Nov 26, 2025

I have not looked into this PR, so cannot comment on the actual change.

What I can say is that I meant this ticket as a high-level usability need. I think designing what we mean exactly by "state" and "time" is necessary before jumping into implementation. This should be grounded on actual use-cases/examples that would clarify what should/could be synchronised.

For example, it stands to reason that keeping the same entity selected (if it exists in both recording) might be relatively straightforward.

What time synchronisation should do much less so. Time ranges in recording might corresponds (e.g. frame 100), or not at all (e.g wall clock time). Should we consider delta from first data? Or something else? This needs figuring out at the conceptual level first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants