IMB: Cleanup timecode options #121001
No reviewers
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
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#121001
Loading…
Reference in New Issue
No description provided.
Delete Branch "iss/blender:simplify-timecode"
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?
Remove options that are duplicate and change description of options, so
they describe bit better, how timecodes are actually used.
Timecodes in Blender have pretty much nothing in common with more widely
known term "timecode". This confused users (and developers).
There were 5 options of which 3 were exactly same. This commit leaves
user with 3 options:
More verbose description was added to the definition in code.
Naming of these timecode types was kept, even if it is incorrect to not
break scripts and habits.
@ -29,7 +29,11 @@ extern "C" {
/* Blender file format version. */
#define BLENDER_FILE_VERSION BLENDER_VERSION
<<<<<<< HEAD
It is a very good idea to scroll through a patch you're submitting for review, to ensure it is actually code you want to be reviewed. And maybe give it a compile test as well ;)
@ -55,2 +55,3 @@
* frames 100 and 102.
*/
IMB_TC_RECORD_RUN = 1,
IMB_TC_INVERSE_MAPPING = 1,
I can not say I fully understand the comment and proposed naming.
Inverse from which observer perspective? it also feels that a less generic term for "mapping" can be used, to make it more explicit that there is no "map" in its programming data-structure is involved.
The "Note" part of the comment seems incomplete to me, as I do not really understand what the message is which it is trying to tell to me.
From just following the comment, a more semantically clear name would be
IMB_TC_MOVIE_TIMESTAMP
.Will modify the comment a bit.
The thing is, that both timecode tables/maps use movie timestamps. So the data is the same pretty much, the difference is: what timeline frame / strip frame these timestamps are mapped to.
I could propose
IMB_TC_NORMAL_PLAYBACK
which would reflect what this is supposed to achieve.1c7b1ee90a
to8f0780db31
@blender-bot build
I find it a bit strange and hard to follow when there is a big difference in naming between UI and code. If the
Use Movie Timestamps
is clear for the UI, it will be clear for the code as well. Similarly, theNo Gaps Playback
is more clear than uniquness of mapping.I am not fully concinced the current change in the name and description makes it more understandable by users. I am also not sure how much time we want to invest into such changes. Ideally we'd get rid of the timecode (which should be doable since all the previous improvements in the seeking code). Until then we can just remove the unused time code options to solve actual confusion, but leave the naming and description of
Record Run
andRecord Run No Gaps
as-is, so we don't break mental memory or scripts which automatically set-up scenes.I still want to remove timecodes completely, though from your comment in !108059 I had impression that it may not happen very soon.
Even if the code was removed, I think that having it in coherent state is useful if somebody would like to look at the removed code when bisecting or for whatever reason.
I can agree on less technically correct naming scheme, perhaps even keeping completely incorrect terminology - using "record run", but I would prefer not to. Anyway, I think that having comments in the enum would be enough to build mental image of what is this code trying to do.
So will keep original naming and further clarify the comments.
I have updated the patch, just a note, that in movie clip editor "Record Run No Gaps" was named "Free Run No Gaps". So I have changed name in UI, but RNA enum is not changed. This is in reference to #120839, which started this whole thing.
@ -67,3 +72,1 @@
IMB_TC_INTERPOLATED_REC_DATE_FREE_RUN = 4,
IMB_TC_RECORD_RUN_NO_GAPS = 8,
IMB_TC_MAX_SLOT = 4,
IMB_TC_RECORD_RUN_NO_GAPS = 2,
Can we keep the original value for
IMB_TC_RECORD_RUN_NO_GAPS
so that the change does not affect on the forward comaptibility?Yes, just will have to remove for loop and hardcode array indices. But this wouldn't complicate the code I would say.
@ -160,3 +155,1 @@
"Free Run (rec date)",
"Interpolate a global timestamp using the record date and time "
"written by recording device"},
"Use for normal playback. Seek based on timestamps read from movie stream"},
Description should be mainly about describing what the option does on a user level, not when to use it. With the current wording it is unclear what the actual difference from None is.
I would say, that it is super clear - Seek based on timestamps read from movie stream vs seek in movie stream based on calculated timestamp.
Did swap the wording, but the fact that Record Run is used for normal playback is important to include, since this is like having to choose between potato peeler and slicer in meat processing plant.
The word "normal" is very subjective, and hence more often than not is to be avoided in favour of using an explicit definition of what you actually mean here. For example: "Seek based on timestamps read from movie stream, giving the best match between scene and movie times".
@ -166,2 +159,2 @@
"Free Run No Gaps",
"Record run, but ignore timecode, changes in framerate or dropouts"},
"Record Run No Gaps",
"This timecode causes each frame procuces new image. This may break AV sync, change "
There are some typos (
procuces
.it's
).Something like "Effectively convert motive to an image sequence, ignoring incomplete or dropped frames, and changes in frame rate" seems to be explaining what's going on in easier terms.
@ -66,4 +72,1 @@
*/
IMB_TC_INTERPOLATED_REC_DATE_FREE_RUN = 4,
IMB_TC_RECORD_RUN_NO_GAPS = 8,
IMB_TC_MAX_SLOT = 4,
It still worth having
IMB_TC_NUM_TYPES
here to avoid hardcoded constants later on.@ -351,25 +344,6 @@ int IMB_proxy_size_to_array_index(IMB_Proxy_Size pr_size)
}
}
int IMB_timecode_to_array_index(IMB_Timecode_Type tc)
You can still have this function, and return an index within a 2-element array. The benefit of sticking to it is that an unexpected/unhandled enumerators will at least trigger an assert.
@ -448,3 +411,3 @@
}
SNPRINTF(index_name, index_names[i], stream_suffix, anim->suffix);
const char *index_fmt;
Do not leave unitniailized and unchecked values. Also, see the comment for the
IMB_timecode_to_array_index
@ -798,3 +770,3 @@
proxy_output_ctx *proxy_ctx[IMB_PROXY_MAX_SLOT];
anim_index_builder *indexer[IMB_TC_MAX_SLOT];
anim_index_builder *indexer[2];
Use
IMB_TC_NUM_TYPES
.From reading the code it seems almost good now!
The
main
needs to be merged, and the version/versioning code updated correspondigly. And after that it will be good from my side.@ZedDB Would you like to have a pass of review here?
@ -3350,0 +3378,4 @@
MovieClipProxy proxy = clip->proxy;
versioning_update_timecode(&proxy.tc);
}
}
I'd keep a new line after the
}
before the comment block for consistency.