Skip to content

Feature/add sessionrecording extension#148

Merged
engbergandreas merged 19 commits intomasterfrom
feature/add-sessionrecording-extension
Jun 10, 2025
Merged

Feature/add sessionrecording extension#148
engbergandreas merged 19 commits intomasterfrom
feature/add-sessionrecording-extension

Conversation

@engbergandreas
Copy link
Member

@engbergandreas engbergandreas commented May 23, 2025

#fixes OpenSpace/OpenSpace#3670

Also select the latest recorded file in the playback dropdown
Fixed some additional issues with the error message so it should be somewhat smoother to use now

Also added option to hide GUI when playing back a session recording: #fixes OpenSpace/OpenSpace#3682 - This one might count as a new feature tho so question is if it should be included now? Can revert the last commit if so.

@alexanderbock
Copy link
Member

Seeing how many people include the UI in their render sequences without intending to, I think it does count as a bug fix

@engbergandreas
Copy link
Member Author

Uncheck hide gui by default

Copy link
Collaborator

@WeirdRubberDuck WeirdRubberDuck left a comment

Choose a reason for hiding this comment

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

Had quite a few things to comment on here. See most of them in code

  1. First, should we maybe hide the extension if the playback file dropdown? It gets much harder to read with all the file names ,and they do not really matter for playback
    image

  2. Do we really need both these checkboxes, or would it be sufficient with one checkbox that says "Hide all GUI elements on playback"? That would make the interface less cluttered, and if a user want to show the dashboard but not the UI I can imagine that they do not want to show all part of it and could go into the settings and hide the things they want anyways? Maybe worth discussing
    image

Comment on lines +118 to +129
<Text mb={'xs'}>
Caution: Enabling both{' '}
<Text fs={'italic'} span inherit>
Loop playback
</Text>{' '}
and{' '}
<Text fs={'italic'} span inherit>
Hide GUI
</Text>{' '}
will cause the interface to remain hidden indefinitely during playback. To
reveal the GUI again, press:
</Text>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see why this text string is needed, and like that the settings are marked in italics, but this text will be a pain to localize using i18next. Did we come up with a good way to deal with italics?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will see what the solution will be in the other PR that handles the translation

@engbergandreas
Copy link
Member Author

Had quite a few things to comment on here. See most of them in code

1. First, should we maybe hide the extension if the playback file dropdown?  It gets much harder to read with all the file names ,and they do not really matter for playback

I see what you mean but since we allow the same name with different extension it will be super weird to see foo twice when one is foo.osrectxt and one foo.osrec

2. Do we really need both these checkboxes, or would it be sufficient with one checkbox that says "Hide all GUI elements on playback"? That would make the interface less cluttered

I think we want both, I could see a situation where I want to show dashboards that could contain e.g., distance information but not show the rest of the GUI

and if a user want to show the dashboard but not the UI I can imagine that they do not want to show all part of it and could go into the settings and hide the things they want anyways?

I think the whole point for these settings is to make it easier to not have to maually do this every time you go playback / normal usage

@alexanderbock
Copy link
Member

How about only showing the extensions if there is an ambiguity? In particular since users would be able to choose whicherever extension they like and not just osrec and osrectxt. Not sure if we need to support it, but if a file has the same basename and only differ in extension, we could show it then, but hide it otherwise

@engbergandreas
Copy link
Member Author

In particular since users would be able to choose whicherever extension they like and not just osrec and osrectxt.

Is this something that we've told users? Because now we specifically check for these extensions and force any new recordings to have these extensions depending on the format used

@alexanderbock
Copy link
Member

Yea, you're right. I don't think we need to account for it. If they give it a wrong extension on purpose, we don't need to take care of it

Copy link
Collaborator

@WeirdRubberDuck WeirdRubberDuck left a comment

Choose a reason for hiding this comment

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

Looks good and seems to work well. And the code is suuper clean now, love it!

My only comment is that it would be nice to not show the extension in the selected playback file either, if possible. I.e. here:
image

@engbergandreas engbergandreas merged commit 7e95323 into master Jun 10, 2025
@engbergandreas engbergandreas deleted the feature/add-sessionrecording-extension branch June 10, 2025 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants