Paths

Table of Contentst

Differential D71

Support SMPTE as input for 'time' fields when unit is set for the scene
AbandonedPublic
Actions

Authored by Dalai Felinto (dfelinto) on Dec 3 2013, 4:37 PM.

Details

Reviewers

Sergey Sharybin (sergey)

Group Reviewers

Animation & Rigging

Summary

If you turn SMPTE as the time unit in the scene settings all the time buttons will
display their value in SMPTE.

You can also input SMPTE values:

1h3s4
1:0:03:42

More on SMPTE:
http://en.wikipedia.org/wiki/SMPTE_timecode

Individual commits log:

Support SMPTE display when entering a button

Add parser to remove leading 0 from smtpe (because blender doesn't understand leading 0)

01:02:03 --> 1m2s3

Add a time unit 'frame' that does not apply any scaling factor

(values entered in the UI are left unchanged)

Added a B_UNIT_DEF_NOSCALE flag to bypass scaling.

Code mirrored from:
https://github.com/dfelinto/blender-git/tree/smpte

Individual commits on:
https://github.com/dfelinto/blender-git/tree/smpte-development

Patch by me and Benoit Bolsee developed for a 3rd party project.

Diff Detail

Branch: smpte

Event Timeline

(adding Animation and Video Sequencer to reviewer since it seems to be the related projects)

Dalai Felinto (dfelinto) added reviewers: Campbell Barton (campbellbarton), Sergey Sharybin (sergey).Dec 3 2013, 4:41 PM

Some quick code comments, didn't try out the patch or think about design.

source/blender/blenkernel/intern/unit.c
424–426	Rounding to integers so early in this function make this not work for fps < 1.0.
428	If fps < 1.0, this gives division by zero, same with the divisions on the other lines.
449–450	This should not be needed, BLI_snprintf should do proper null termination of the string (unlike sprintf). Also the {} brackers for this BLI_sprintf call are not needed.
659	Use `bool found_smtpe = false;` instead.
source/blender/editors/interface/interface.c
1575–1579	This can be removed instead of #if 0'd?
source/blender/makesdna/DNA_scene_types.h
1121–1124	Could use a single "char pad[7];" for padding.

smpte: addressing 1st review round

I don't see the harm in it; for animation it is customary to think in frames but perhaps video editing folks would like this.

Mostly a code-wise review, @Campbell Barton (campbellbarton) is the reference here about general code design and API. :)

Usability (user side)

Good thing to have (would even use it for the modal numinput D61 patch, we have a few time-like cases here). Noted two glitches, though:

Negative values are wrong (a neg sign before each hour/min/sec/frame block, see e.g. the main frame numbutton in Timeline window).
When the numbutton is too small to draw the whole string, it shortens the 'min' part first (it simply clip everything before the last ':', things may have changed with Campbell's latest work here, but I suspect getting a correct behavior will still be tricky!).

General Design (code side)

Disclaimer about this part: I’m not the maintainer of unit system, you should probably wait for Campbell's advice here!

There are two points that may be issues, as far as I can see:

ui_is_but_smpte()

This func seems to be used to force a specific 'smpte' parsing. However, we can e.g. enter imperial distances even in metric system, they get interpreted just nicely. Imho, we should also be able to enter either 'frame' or 'smpte' values, independently of the chosen time system. In which case, imho checking that the but's unit is TIME should be enough.

bUnit_ToUnitAltName_smpte()

I’m pretty sure at least the name is wrong (bUnit_ToUnitAltName() is only used to replace 'funny' non-ascii units codes (like µm -> um), not format a raw value into 'united' one).

In fact, there should be no need at all for this function, rather the end of ui_get_but_string_ex() (around interface.c:1920) should be modified to also allow unit handling when button is integer.

source/blender/blenkernel/intern/unit.c
94–97	Should be changed to "nice" bit-shift (1 << 0, i << 1, etc.) on the move! ;)
427–428	This looks far over-complicated to me? h = (int)floorf((float)value / frame_in_hr); Should do the trick imho (same for minutes and seconds, of course). Or you stay in double and use floor() and double frames_in_hr & co. And we would get rid of 'unused var' warning.
562	Shouldn’t it still use unit->scalar here?
632	smpte should be a bool, but don’t think it’s even needed, see my main comment.
670–673	I would consider this as a bug in current code (actually, it's python that does not like leading '0')… Will try to fix this in another patch, imho should not be handled only for timecode.
678	Beware of our code style, one space before and after operators, and "else" on a new line, etc. ;)
source/blender/editors/interface/interface.c
1571–1580	Do we really want to consider "frame" time system as a unit one. I would rather handle it as we do with radians (hence only return true here when time system is SMPTE).

Note: removing heading zeros: D119.

Update for latest trunk (fixing conflicts after numinputs commit - 87cc890aef53c4660448b1125dc0c40a187ae1f2 )
Note: I'm waiting for a reply on D61 but most likely I will need to make bUnitString_AsString_smpte into size_t

This is a bit of a hack of unit.c with an exeptional case for the bUnit_ReplaceString(..., smpte)

I really dont think this is the way to go, time handling is fairly different to units and would better just go into its own functions.

There was discussion recently of adding BLI_timecode.h to have related functions, possibly functions to parse and convert timecode could go there.

Then the Interface code can check if timecode buttons are used and parse them differently.

What is the status on this?

Campbell Barton (campbellbarton) removed a reviewer: Campbell Barton (campbellbarton).Aug 11 2016, 6:14 AM

Aaron Carlisle (Blendify) removed a reviewer: Video Sequencer.Dec 29 2016, 9:59 PM

Aaron Carlisle (Blendify) added projects: BF Blender, Video Sequencer.

Even though this is an old patch, the need for it is still present. Ex. take a look at the ugly Time sidebar panel in the VSE for example. Having either SMPTE or frames in the widget and being able to type in SMPTE values would solve this.

This revision now requires review to proceed.Jan 29 2020, 6:25 AM

@Peter Fog (tintwotin) This would be in line of what I was thinking about doing in VSE. so thanks for bringing this to my attention.

Not sure if there was any discussion "behind the scenes" there, I can imagine this wouldn't be so straightforward to implement for all time fields in all areas.

@Richard Antalik (ISS) there was no discussion. I made this patch for a project that I worked on, but it never made to Blender. Feel free to take from here any time.

Scott Wilson (propersquid) added a subscriber: Scott Wilson (propersquid).May 14 2020, 3:12 AM

Peter Fog (tintwotin) added a project: VFX & Video.Sep 22 2020, 8:18 AM

I'm abandoning this patch. If anyone wants to pick it up it should be easy to find it again. But I'm no longer actively working on this. (haven't been for 7 years already)