Page MenuHome

Support SMPTE as input for 'time' fields when unit is set for the scene
Needs ReviewPublic

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

Details

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)

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

source/blender/blenkernel/intern/unit.c
428–430

Rounding to integers so early in this function make this not work for fps < 1.0.

432

If fps < 1.0, this gives division by zero, same with the divisions on the other lines.

453–454

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.

658

Use bool found_smtpe = false; instead.

source/blender/editors/interface/interface.c
1571–1575

This can be removed instead of #if 0'd?

source/blender/makesdna/DNA_scene_types.h
1128–1131

Could use a single "char pad[7];" for padding.

Dalai Felinto (dfelinto) updated this revision to Unknown Object (????).Dec 10 2013, 10:39 PM

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–98

Should be changed to "nice" bit-shift (1 << 0, i << 1, etc.) on the move! ;)

431–432

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.

568

Shouldn’t it still use unit->scalar here?

638

smpte should be a bool, but don’t think it’s even needed, see my main comment.

669–672

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.

677

Beware of our code style, one space before and after operators, and "else" on a new line, etc. ;)

source/blender/editors/interface/interface.c
1567–1576

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.

Dalai Felinto (dfelinto) updated this revision to Unknown Object (????).Dec 23 2013, 5:00 PM

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

Campbell Barton (campbellbarton) requested changes to this revision.Jan 20 2014, 1:15 PM

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.

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.