这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@davidlamhauge
Copy link
Contributor

This PR adds a label on the statusbar, that shows (1) No text, (2) Frame number or (3) Time code, for the scene.
I've put a ToolButton in the statusbar as well, so you can change your info type on the fly. The preferred info type is saved in the preferences.
For Time code, I've only MM:SS:FF and not Hours. Since we can have maximum 9999 frames in a scene, and 9999 frames at 3 fps equals less than an hour (55 minutes), I reckon that Hours is not needed, but it can easily be added.
David_DavidScreencast_20200812_17_44_19

I realize that this solution differs somewhat from the feature suggested in 935, but if it is accepted it addresses #935

@Jose-Moreno
Copy link
Member

@davidlamhauge Nice job David! Sorry for not commenting much on your PR's and contributions lately, I've been very busy 😖

On the matter of MM:SS:FF, I understand why you took out the hourly digits out, though I've seen programs that simply leave it with a single H digit as in H:MM:SS:FF when it's not sure to reach hourly long footage so e.g 10 seconds at 30FPS would be -> 0:00:10:30

Well I don't think it's a big deal anyway as long as people understand what is it they're looking at, and right now it's at least clear which system is being used 👍

On a more technical level related to usability, would it be possible to get rid of the little dropdown box and make the label itself behave as the selection menu instead? I think clicking on the timecode itself would be nice. If that's not possible, I suppose changing the icon would help to make it more obvious that you can change the timecode format, otherwise it looks as if you can change the zoom% instead (which would also be nice! I'll ask for that later 😆 )

Other than that I think it will be a welcome addition. Thanks for looking out for all of these issues! 😊

@davidlamhauge
Copy link
Contributor Author

Thanks for the feedback, @Jose-Moreno !

About a digit for Hours, I'll wait and see what the other think. I don't think it's needed.

The tool button in the statusbar is there, because I was afraid that we would enter a 'How to change camera size'-problem, if it wasn't a noticable button. I do see, that if you choose 'No text', it will look like the button is about the zoom, so maybe there should be three slashes '---', periods '...' or something else, to indicate that there in fact is a label.

@scribblemaniac
Copy link
Member

scribblemaniac commented Aug 14, 2020

I have not had a chance to test or look at the code yet, but I'm liking the way this looks. I did not even know you could add interactive widgets to the status bar.

I agree that hours aren't really necessary, however I would use the format MM:SS.FF (note the period instead of the last colon). This is how ffmpeg, and Audacity do it. Kdenlive uses a comma or a semicolon (might be local specific too). All these solutions help helps distinguish MM:SS:FF from HH:MM:SS.

I would also remove the separator between the button and the frames/timecode text to help emphasize that the button is for that segment. I think the button might also be better on the right, since that mimics the location of arrows on a combo box. That is OS-specific however and there is something nice about the button not moving when you change settings, so probably just leave it unless everyone else things the right side is better.

@scribblemaniac scribblemaniac added Enhancement UI Related to the visual appearance of the program labels Aug 14, 2020
@davidlamhauge
Copy link
Contributor Author

I've replaced the last colon with a period. It looks good.
There is no separator in the status bar. There is a zoom label and a timecode label. I've tried to remove the visible border, but haven't been able to find the solution yet.

@Jose-Moreno
Copy link
Member

@scribblemaniac @davidlamhauge Excuse me, but that shouldn't be changed, the semicolon is proper in this case, please read this quote from the wiki on SMPTE Timecode.

While non-drop timecode is displayed with colons separating the digit pairs—"HH:MM:SS:FF"—drop-frame is usually represented with a semicolon (;) or period (.) as the divider between all the digit pairs—"HH;MM;SS;FF", "HH.MM.SS.FF"—or just between the seconds and frames—"HH:MM:SS;FF" or "HH:MM:SS.FF

Since Pencil2D is not dropping frames (we don't use floating point FPS) it should use the colon previous to the last commit. You also see this in programs like After Effects:

image

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Aug 15, 2020

@Jose-Moreno I know that colon is the 'proper' way to separate in timecode. That's why I used colon in the first case.
I didn't know that period and semicolon was used for drop frame timecode.
After seeing period as a divider between seconds and frames, I kind of prefer that to colon. It is like it divides time-units and movie-units, but at the end of the day, I don't care. I'll put period or colon - what ever you guys can agree upon.

@J5lx
Copy link
Member

J5lx commented Aug 17, 2020

Nice to see this implemented! I don’t have the time to have a thorough look at this right now, but based on the screenshot there’s one tiny detail that I would prefer to be changed (other than what’s been discussed so far), namely the alignment. The convention for status bars is to put permanent stuff on the right and temporary stuff or contents that change often on the left. Left-aligning the zoom level made sense previously because it was only a “temporary” message (which was simply very rarely overwritten), but now with the truly permanent widgets the right side would be a better fit. The reason I’m bringing this up is because I also got some status bar improvements in the works which make use of the left area (and also an interactive zoom widget, which I believe someone mentioned above).

Nice work David!

@scribblemaniac
Copy link
Member

Fair point about the drop frame rule, however I should point out that MM:SS:FF is not proper SMPTE either as it's missing the hour section, which is definitely unnecessary here. Personally I still think there should be different separator to distinguish time units from frame units.

Copy link
Member

@J5lx J5lx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Time for a review! A few smaller issues, but nothing too serious. As a general note, I’d prefer something more descriptive like “timecode format” over “timecode text (level)” or “timecode label enum” in the code.

For the timecode format, I agree more with the use of a colon. I’m not sure if I’ve ever seen a dot used for that in the context of animation, so chances are our users too are going to encounter the colon notation elsewhere. I’d say it’s better to get used to that early – once you know the meaning, the dot notation isn’t much of an improvement anyway (IMO).

Lastly, in the original issue it was recommended to include both the SMPTE and the FFF (S:ff) timecode as options. Considering the latter’s use in Japanese animation, it seems worth adding that to me.

@davidlamhauge
Copy link
Contributor Author

I think I have addressed all issues now.
I didn't know the Japanese time code S:FF, and tried to look it up, but couldn't find anything. I can't see the logic in calling it FFF, so I've called it SFF right now, but it can easily be changed, if it indded is FFF.

@Jose-Moreno
Copy link
Member

@davidlamhauge I think it's fine with what you have. Perhaps that japanese counter shown before was overly specific to some studios so it's not a problem.

If you see Opentoonz Timecode Iwa Effect they only have absolute frames and SMPTE (which is widely used in Japan and the Americas along NTSC)

image

@davidlamhauge
Copy link
Contributor Author

I have included the Japanese S:FF mode, and moved the info to the timeline, so we have four options: No text, Frames, SMPTE and SFF.

Copy link
Member

@J5lx J5lx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m still unhappy about the implementation of the format selection menu and the TimeControls layout can use some tweaks. The other comments are basically just about cleanup, nothing serious. Once those things are taken care of, this PR is good to go!

Copy link
Member

@J5lx J5lx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here’s what I meant wrt the menu.

Edit: GitHub just had to mess up the order of my comments. This one was meant to be go along with the code suggestions above.

Copy link
Member

@J5lx J5lx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, merging.

J5lx added a commit that referenced this pull request Oct 28, 2020
@J5lx J5lx merged commit 0df037d into pencil2d:master Oct 28, 2020
@J5lx
Copy link
Member

J5lx commented Oct 28, 2020

@Jose-Moreno @candyface Shall we treat this as a fix for #935 as suggested in the OP or leave that issue open until we have an actual on-canvas display?

@MrStevns
Copy link
Member

MrStevns commented Oct 28, 2020

Hmm.. It's not quite the solution I had in mind for #935 but it does solve the problem. Alright let's close it.

@Jose-Moreno
Copy link
Member

@J5lx I feel the same as CandyFace. I suppose we can close it then. I'll do it after organizing the Issue via the project board to be related to this PR then. Thanks to everyone involved in this PR.

@Jose-Moreno Jose-Moreno linked an issue Oct 29, 2020 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement UI Related to the visual appearance of the program

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Display frame counter on canvas

5 participants