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

Timelapse thumbnails #4370

Merged
merged 10 commits into from Jan 24, 2022

Conversation

crysxd
Copy link
Contributor

@crysxd crysxd commented Jan 16, 2022

What does this PR do and why is it necessary?

This PR adds an additional step when rendering a timelapse to also generate a thumbnail from the last frame of the timelapse. This is handy to see the final state of the print, making it easy to identify a timelapse. Thumbnails are completely optional, e.g. if we fail to generate a thumbnail we do not fail the overall render process. Generally thumbnails are optional to maintain backwards compatibility with previously rendered timelapses as well as old clients.

The thumbnails piggy back the existing storage and download mechanic for timlapses. They are stored in the same folder and share the same filename as the timlapse with the .thumb.jpg suffix. This means the same access permissions apply and they share the same E-Tag validation for caching.

The timlapse model now contains an additional (optional) field thumbnail with a download URL in the same scheme as the actual timelapse file.

In the UI the timelapse tab was modified to show the thumbnails. The design on the list was updated to more closely align with e.g. the file list. This means the actions are now mini buttons. The "preview" button was removed and the user can now click the thumbnail to open the player, this is indicated by a "play" symbol overlaying the thumbnail. A second new field timestamp was added to the model in order to include the date of the file as timestamp. This is used to show the recording date in the UI in the same style as for file upload times.

How was it tested? How can it be tested by the reviewer?

This was tested on a macOS and OctoPi host. The UI changes are tested on Safari and Chrome.

Any background context you want to provide?

/

What are the relevant tickets if any?

/

Screenshots (if appropriate)

Screenshot 2022-01-16 at 15 16 53

Further notes

The images in the UI use the native lazy loading mechanic of the browser. This is not yet supported by Safari (but available in technology preview and confirmed working as intended), which means the images of the first page of the thumbnail list load with the page. As the images come with an E-Tag, subsequent page loads will not load the images again as OctoPrint returns 304. My hope is that the lazy loading mechanic will be part of the next major version of Safari as it's already working fine in TP.

I did not update the documentation yet as this would cause potential conflicts with my other PR. My plan is to merge #4349 first and then update this PR with the documentation. My idea was to keep the two PRs separate as they have different scopes and goals and I expect this PR to be "harder" to merge than the other one :)

@github-actions github-actions bot added targets maintenance The PR targets the maintenance branch approved Issue has been approved by the bot or manually for further processing labels Jan 16, 2022
@crysxd crysxd changed the title Improvement/timelapse thumbnails Timelapse thumbnails Jan 16, 2022
@foosel foosel self-assigned this Jan 17, 2022
@foosel foosel added this to the 1.8.0 milestone Jan 17, 2022
@crysxd
Copy link
Contributor Author

crysxd commented Jan 21, 2022

Small question for debate: Is the thumbnail too large in the list? Using 170px atm, but the longer I look at it, the more I feel maybe 140px would be better...

170px

Screenshot 2022-01-16 at 15 16 53

140px

Screenshot 2022-01-16 at 15 16 53

120px

Screenshot 2022-01-16 at 15 16 53

@foosel
Copy link
Member

foosel commented Jan 21, 2022

Hm... I agree 170px is too big. I'm now actually leaning towards 120px though with this preview 😅

@crysxd
Copy link
Contributor Author

crysxd commented Jan 21, 2022

Agreed 😅 will update. Below 120 becomes too small IMHO, but I can post a preview of you want

@jneilliii
Copy link
Contributor

Yeah, 120 looks proportionally right for default interface. Would look different with wide screen theme though.

@crysxd
Copy link
Contributor Author

crysxd commented Jan 21, 2022

Done! I set it to 110px now, with this the height for 16:9 roughly matches the text of the details. Also the list is not as long anymore compared to the files on the left:

Screenshot 2022-01-16 at 15 16 53

Copy link
Member

@foosel foosel 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! Before I merge this, only one question - should we maybe also add some kind of "rerender thumbnail" command, for existing timelapses? Maybe even bulk/"render all missing" or something?

@crysxd
Copy link
Contributor Author

crysxd commented Jan 24, 2022

I think this is an interesting idea, I also briefly thought about it, but I'm not sure if it's smart to do.

  • We have a potentially large number of timelapses which might result in a long computation time after the update, making the user wait to start prints
  • I'm not sure if people actually look at old timelapses that much, I personally don't delete them and don't care as I have a 128GB SD card, but I don't look at them. My assumption would be that this is similar for most people, reducing the benefit
  • As all new timelapses will have thumbnails, this "problem" solves itself over time

Not sure if you agree, but for the above reasons I'd keep things simple! I have an idea though on how to visually improve the "no thumbnail" state, will play around tonight

@foosel
Copy link
Member

foosel commented Jan 24, 2022

I agree that this would be an issue if we force it automatically on start or something. I was rather thinking of giving the user the option to trigger it for one/some of the timelapses.

Merging for now however :)

@foosel foosel merged commit cac56cf into OctoPrint:maintenance Jan 24, 2022
@crysxd
Copy link
Contributor Author

crysxd commented Jan 24, 2022

Oh, then I misunderstood your idea. I'll check if it's easy to do and make a separate PR if so :)
Will also update the documentation PR with the new thumbnail field

@crysxd crysxd deleted the improvement/timelapse_thumbnails branch January 24, 2022 14:17
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Issue has been approved by the bot or manually for further processing targets maintenance The PR targets the maintenance branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants