VSE: Make cache overlay visible to non "Developer extras" users #119428
No reviewers
Richard Antalik
Pablo Vazquez
Sergey Sharybin
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
5 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#119428
Loading…
Reference in New Issue
No description provided.
Delete Branch "ZedDB/blender:vse_cache_overlay"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This also moves the option to be a per editor space setting so you can
have different cache visualization options in different editor spaces
at the same time.
A quick toggle for the cache visualization is now available in the
overlays popover.
To follow the cache settings in the timeline, I enabled all cache options. But perhaps we want to only show the "final image" cache per default? Note that is easy to toggle this on/off now in the overlay settings, so perhaps having everything on is ok? The additional cache options shouldn't occlude anything as thumbnails are off per default.
194f453a5ato6ed860979cI think, that the idea was to only show final images without developer extras. For normal users it would not be of any benefit to see other types I think.
From code perspective, seems OK. I would probably move cache tab from sidebar to overlays panel, so you have all stuff together. If this is hidden by developer extras it could be 3 joined buttons for "cache extras".
@ -812,3 +809,1 @@SEQ_CACHE_VIEW_PREPROCESSED = (1 << 7),SEQ_CACHE_VIEW_COMPOSITE = (1 << 8),SEQ_CACHE_VIEW_FINAL_OUT = (1 << 9),/* Deprecated: CACHE_VIEW setting has been moved to CACHE_SHOW in DNA_space_types.hI would just remove these definitions, especially if there is no versioning.
I left them in here in case someone tries to reuse them. If I just delete them, I think it might be easy to forget to clear these in the versioning code.
Or perhaps I should just delete these and add a clear entry in versioning right now?
@ -646,6 +646,20 @@ typedef enum eSpaceSeq_SequencerTimelineOverlay_Flag {SEQ_TIMELINE_SHOW_GRID = (1 << 18),} eSpaceSeq_SequencerTimelineOverlay_Flag;typedef struct SequencerCacheOverlay {Not sure if it is necessary to make new struct for this. Functionally and semantically it would be just as fine under
SequencerTimelineOverlay.I made a new entry as the
SEQ_TIMELINE_flags would only have 8 entries left before it would be full. I think this might also make it more organized if we for what ever reason need to add more cache options in to future.Do you have any strong opinions about this?
Not really a strong opinion. But bits 10-13 are free,
SEQ_CACHE_SHOW_FINAL_OUTcould be implied bySEQ_CACHE_SHOW. so this is squeezable :) I would do it...Ehh... I don't really like to try to squeeze things into other places if it is easy to just do a clean struct that has a clear category and intent. Especially because this area might change a bit in the future.
It is possible to share this cache by multiple copies of object before first change?
The description could use a motivational part about why artists need to worry about something that hd a clear intent of a developer-only functionality.
As for the level of caches, I do not really see why artists will worry about anything but the final images. So at a very least, the other caches are to be disabled by default.
But I'd also go as far as saying that for the artists without Developer Extras there should be a single "Show Cache" option. The granularity of levels could then be done and shown when Developers Extras option is enabled.
@ -341,3 +342,3 @@col = layout.column()col.enabled = ed.show_cachecol.enabled = cache_settings.show_cacheUse
activeinstead ofenabled. Theenabledis only to be used in cases when changing property is not possible at all, without causing some serious side effects (like, crashes). If the property contextually is not used, then artists should still be able to modify the value.It similarly, say, how you can tweak simplification settings in the Simply panel without actually enabling simplification.
I was giving more testing to the patch, and now I am definitely convinced with the current state of the cache visualization anything but the final-image cache is definitely a no-go.
Looking at the current edit of the project:
That per-strip visualization is going to interfere with thumbnails, possibly waveforms, missing media visualization, retiming, and possibly other things which do not pop up in mind immediatelly.
Additionally, the visualuzation of cached for metastrips seems to be broken:
Marking as requested changes.
I also noticed yesterday after posting this PR that at least the other cache options are at least broken visually. For example if you enable the cache for composite and preprossessed images, it seems to work when you first fill up the cache.
However if you then move the strips, the cache lines doesn't move to the correct frame.
I looked at the code and the reason for this is that the cache stores the absolute timecode for the cache and uses this to visualize where to draw the cache lines.
This works fine for final image caches, but not for per strip caches.
The code does store the actual relative frame of the strip as well, I think this might be an easy fix. I'll add a commit with the fix here soonish.
I do agree that we probably only want to have the final image cache option on per default.
I don't think hiding the extra options behind developers extras is a good idea. Especially because users can the get files with the options turned on forcing them to either go through python or enabling the developers extras to turn it off.
The options are already hidden away enough as it is I feel.
The reason why we are moving this out of developers extras is that end users would be very confused why their playback performance were so inconsistent. They didn't think Blender had any caching as we hid away the options for it. So instead they just thought Blender was very crappy as it would playback sections fine sometimes and sometimes it would stutter like crazy.
With the ability to see the cache, they can now tell that the stuttering is because the cache is not populated.
Look into how, for example, Debug panels work in Cycles. There are a lot of options exposed there which could totally change certain algorithms, and they are saved and loaded into .blend file, but only have effect if the Developers Exrta + Cycles Debug is enabled. All the low-level debugging functionality in Sequencer should behave the same.
Having cache exposed at all is already quite a stretch, and is only acceptable because there is no time from a capable developer to solve the playback issues. Having a single cache display has a very simple mental model: frame cached == fast, frame non-cached == hiccups. I do not see any reason why regular (even highly experienced) video editors would need to worry about all those lower-level and technical details of extra levels of caching.
Saying that option is hidden enough is not a good excuse for compromising quality and clarity of the UI/UX.
I've disabled the drawing of the strip (non final) caches unless developer extras is on. I also made a quick fix to improve the drawing of the strip caches. They now move with the strips correctly. However they are still broken for meta strips and video strips with re-timing etc. As we only show these for developers, I opted not to work on that part any further as it would require quite a bit of extra code.
I'm waiting for Richard's replies before I act on his feedback.
When a video editing project is created from scratch the option behaves very close to what I'd expect it to. At least on the menu/option level. It also gave a chance to play around in more details, and see how it feels and behaves.
The drawing itself could become but more about it later.
Opening an existing file and enabling cache display does not actually display cache. I guess it is because some code only initializes individual caches for new files, and drawing code relies on it, even when developer UI option is disabled.
Now, back to the UI and drawing.
The simple one: the option in Overlay menu needs to be called
Cache(not plural).The readability of the cache line could become very bad very quickly. Check the scheenshot:
The quick solution I can think of is to move the cache line to the top, and make it non-overlap drawing. Also, have it fixed size in UI pixels
(reglardless of the zoom level).
There is also an issue when horizontal zoom is small: it might be hard to see a gap. This needs a separate solution, but it is not something that people brought up with a cache line in Clip or Image Editor, so perhaps is not that important.
d0957c8a2bto3c87ecbbdeI moved the cache drawing to be on top of the timeline:
Still waiting for Pablo to provide feedback if he has any
I would try to make height a bit thinner, since it is quite prominent in such thin region, but not sure if it would be better. Perhaps different color could be used to make this less prominent, but still visible.
@fsiddi @pablovazquez Are you checking on this from the design perspective?
This is 1/10 of scrubbing region height. I like it a bit more, since current frame indicator does not intersect it
Tried gray color as well, but I don't think this is great:
On that note, since this is visible by default now, it would be good idea to have a theme for these colors.
No need for even more theme options. For over a decade we have the concept of cached media in the Movie Editor and Clip Editor.
We can simply re-use that. These colors have always been hardcoded and it hasn't really been a problem. If it becomes a problem one day we could add a global setting.
The cache line should not overlap. It should add padding to the top of the time scrubbing region, pushing all content down slightly. The following mockup uses 4px (twice the width of the current frame indicator line).
For the record, we discussed this in person and agreed on trying out the cache lines under the time-scrub area instead (mainly for convenience and future proof with more cache types).
I did a version last week with the cache line under the scrub area:
It does look ok when zoomed in. But when I zoom out, I feel like it kinda blends into the the rest of the UI:
Of course this is probably a contrived case, but I just wanted to exaggerate the effect to make my point more clear.
Probably nothing that we could fix with some themeing, but I wanted to check in what other are feeling.
Looks good! +1
It doesn't bother me tbh. In a screenshot it might look a bit confusing but while actually using it the cache line is fixed in place, so it's pretty obvious what is Blender's UI and what not because it zooms in/out with the rest of the content in the timeline.
3c87ecbbdeto7f71deb4edJust tested it, visually it works. How do you turn it on though?
Tested on factory startup. The only way I found to enable the cache is by:
Developer ExtrasShow Cachefrom theView→Cachemenu.Final Imagesfrom theCachemenu.The
Show Cacheentry in theViewmenu should be always available regardless ofDevelopers Extras, and turned on by default. All the options inside theCachemenu I'm not sure I leave it to you developers to decide. But theFinal Imagesoption should be on by default at least? Otherwise it takes opening the menu twice just to see anything.Talked a bit more with Pablo.
We will move the "Developer Extras" Cache panel to the n-panel so that both the cache drawing and settings is in the same place.
Without developer extras, only the "Final images" view toggle will be available and it will no longer be implicitly tied to the overlay "Show cache" option when not in "Developer Extras" mode.
Okay I missed the fact that
Cacheis now an Overlay setting.Thinking about it again, these options don't belong in the
Viewmenu. AllCachesettings have their own tab on the sidebar, and their own panel. Display settings should be under aDisplaysubpanel of theCache Settingspanel.The new
Displaypanel should be always open (there are so few panels in the sidebar anyway), and always available regardless ofDeveloper Extrasbeing on. No need to hide away settings that are considered important. If it can be toggled (by the setting in the parent panel), its display should be toggleable too.If cache types like
CompositeorPreprocessedare too advanced/technical/debug they could be hidden viaDeveloper Extras, but not Final or Raw perhaps?Mockup:
There is a checkbox in the header that basically toggles the
show_cacheoverlay. Also sorted the cache types list so the most important for the user (Final) is first.Awesome! Works as expected now.
One last thing. Since cache is off by default in old files, it might be a good idea to at least do versioning to turn on
Finalcache on old files. Otherwise enabling the overlay doesn't seem to do anything unless the user goes to the sidebar and enablesFinalmanually. A bit cumbersome.How it looks at the moment on an older file:
Other than that I'd say it's good to go.
I think it will be indeed very good to enable Final cache display for the current files. From user perspective it was a hidden option, which was disabled by default. And with the current state of PR there is the visibility that an overlay option does nothing.
@ -829,3 +826,1 @@SEQ_CACHE_VIEW_PREPROCESSED = (1 << 7),SEQ_CACHE_VIEW_COMPOSITE = (1 << 8),SEQ_CACHE_VIEW_FINAL_OUT = (1 << 9),/* Deprecated: CACHE_VIEW setting has been moved to CACHE_SHOW in DNA_space_types.hFor those it works better to clear the flag now and rename them to
SEQ_CACHE_UNUSED_{5,6,7,8,9}. See, for example,WO_MODE_UNUSED_1 = 1 << 1, /* cleared */.No need to refer to something that was there in the code, but not any more. The comments are about the current state of the code.
d407694e4cto23a510c273@blender-bot build
The lint and windows faliures seems unrelated to my changes from what I can see...
Indeed I don't think the failed tests are anything to do with this PR.
I've left a small comment about code comment, which, I think does not really help and better be removed for clarity. The rest seems to work as expected to me now.
So if you can remove that comment, think you can go ahead and land. No need to wait an extra iteration of review for that,
6a0758e294to5cb1385b3c@blender-bot build
@blender-bot build