Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Timeline CSS: More visible keyframe marks #3842

Merged
merged 3 commits into from
Jan 27, 2021

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Nov 17, 2020

This PR makes a small tweak to the Timeline CSS, with the goal of making the keyframe position markings (the small green ticks along the lower edge of the clip/transition) slightly more visible.

  1. It shifts the marks slightly to the right (by 2px, since I noticed marks at the first frame had slid slightly beyond the edge of the canvas for clips positioned at the far left of the Timeline)
  2. It widens each mark from 1px wide to 2px wide, ensuring that a mark can't disappear completely behind the playhead line when they're both positioned at the same frame.

Before:

Screenshot from 2020-11-17 01-12-57

(Granted, the point where a mark could disappear behind the playhead line entirely wasn't actually the same position as the keyframe, indicating that the positioning issues went even deeper:)

Screenshot from 2020-11-17 01-12-40

Nevertheless, both problems are solved in the After:

Screenshot from 2020-11-17 01-08-38

Screenshot from 2020-11-17 01-08-48

@rexdk
Copy link

rexdk commented Nov 19, 2020

This is great, thank you. And educational

I'm sure you've checked, but just in case, does a keyframe at the end of a clip still show?

@rexdk
Copy link

rexdk commented Nov 20, 2020

Been enjoying looking at this. My goodness, the code reads like "hey, how many different software technologies can we squeeze into one app!" It does explain why when I open OpenShot I can go and make a cup of coffee while it gets started.....

  1. .point_region: The fact that margin-left: has to be set negative (presumably allowed by Angular) suggests that perhaps the maths is wrong in index.html? But I guess not to worry if it works.

  2. .point_icon: useful to have it 2px wide, visible behind the playhead. Another option would be to have it 3 px wide, and use borders to create a down arrow (standard if old-fashioned technique). Don't know whether a pretty alpha gradient could be implemented though. However it has the advantage that the icon is visible both sides of the playhead, and it's a lot more obvious in general. The current 'tick' is really an apology for the keyframes - "well, they're there, but we hope you don't find them..."

Just an option - ignore if not helpful

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Nov 22, 2020

@rexdk

I'm sure you've checked, but just in case, does a keyframe at the end of a clip still show?

It does, though it's dangling perilously over the edge.

image

Fortunately, IIRC with absolute positioning overflow:visible is the default. (Sure seems that way.)

  1. The fact that margin-left: has to be set negative (presumably allowed by Angular) suggests that perhaps the maths is wrong in index.html?

AngularJS wouldn't be the concern, it's WebKit that controls the DOM. But negative margins are a staple of HTML layout since the invention of the DIV tag, pretty much. Their primary use is exactly how we're using them: To push things back out over their borders. The keyframe point divs are child nodes of the div.clip_top, which in turn is a child of div#clip_{{clip.id}}, so the origin for their placement would be several pixels inside the clip margins. To push them outside so they stick out not just to the edge but over the clip's borders, even (which in turn lets us draw them at the same scale as the clip itself), they get a little negative-margin realignment.

The math in the HTML then determines how far from the first frame-1 tick(s) any later ones will be placed, based on those equal-scale dimensions. (The fact that they end up slightly over in both directions is a problem with our clip HTML/CSS. At one point I had a fix for all that in #3617 / #3618, but unfortunately neither PR ever made it in.)

2. Another option would be to have it 3 px wide, and use borders to create a down arrow (standard if old-fashioned technique). Don't know whether a pretty alpha gradient could be implemented though. However it has the advantage that the icon is visible both sides of the playhead, and it's a lot more obvious in general.

Hmm. It's not impossible, certainly, but... I kind of wanted to keep them as small as possible, since they can be perilously close to trampling all over each other at even the closest zoom, with a high enough project frame rate.

I went back and forth a few times on whether I should even do this. Thanks to @vincentdavoust in #3402 we can finally navigate to the points (which has made a huge difference), and arguably it means we really don't even need to see them anymore. I have a feeling most users don't need to, anyway — it was annoying me not being able to see them while debugging keyframe code, but that's kind of a special case.

When I initially submitted the PR (#3039) for the content-based rendering code (previously the marks really were tiny little PNG icons), I mentioned the possibilities that arose from drawing the marks as page elements, including possible future interactivity: My first version of the PR had commented-out code to make the marks grow huge on mouseover:

...But that was before #3402, and if anything I feel like there's less reason for any of that sort of thing now... unless the marks were in fact to become mouse-interactive, at which point they'd definitely have to present a bigger target. But I don't feel like we're close enough to that point yet. Though maybe someone will surprise me, #3402 certainly did.

@rexdk
Copy link

rexdk commented Nov 22, 2020

It does, though it's dangling perilously over the edge.

Thanks - I realised I could test that for myself by editing the css, and had just found the same thing. And hey now I can see the first keyframe :)

This is all very instructive - thank you. #3402 has made a huge difference to me too.

I did just wonder if the first keyframe was hidden deliberately - so the dude doesn't see it on every clip when no animation is in use. This is going back yet again to the lack of (need for) a 'no-animation' option :)

@rexdk
Copy link

rexdk commented Nov 28, 2020

Just a heads up that if there is a clip butting up to the right hand edge of this clip, the final keyframe of this clip (overhanging) can become invisible/ overwritten.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Dec 1, 2020

@rexdk Well, to some extent that's always going to be an issue, the same way you can't see any keyframes past the point where another clip or transition overlaps the current one.

Hmm. Unless... We have all that gutter space underneath the clip, what if we use it?

What do you think of this? (There's a last-frame keyframe on the selected clip.)

image

In the butted-up-against-each-other case, that would look like this:

image

@rexdk
Copy link

rexdk commented Dec 2, 2020

Ah yes, that works, thank you. I did try hard to understand the maths but got lost in the layers :) Not to worry

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Dec 2, 2020

@rexdk Part of the issue is, I'm coming to realize, that our clips are all drawn one frame too short. Someone mentioned that in a bug report a while back, and I'm coming around to agreeing with them. (Heck, maybe it was you.)

If you think about it, when the Playhead is sitting at the END of a clip, its last frame is still visible. It's at the START of that last frame. Which means the frame, and therefore the clip, doesn't really end for one more frame-length of time. That's part of what weirds a lot of our visual interactions on the Timeline, I suspect.

(Though it's not as bad as the fact that things magically change size at random when you drag them. I really need to redo the PR that fixed that, I tried resurrecting my old branch but it was too out of sync with develop.)

@rexdk
Copy link

rexdk commented Dec 8, 2020

Sorry for the delay, Christmas stuff. I've again failed to understand the code properly - but I don't need to. The whole thing is written in the most complex way, and doubtless you're right that there are fundamental issues. On the positive side, I guess they are mainly only evident to power users. I was aware of sizing and playhead positioning issues on my last project, but ignored them.

I guess it's another of these 'devil is in the details' situations.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Dec 8, 2020

@rexdk Oh, much of the timeline JS is just... incomprehensible. It's doing all sorts of magical stuff for itself that it should just be leaving to JQuery, and often doing it slightly wrong. I went looking for the resize bug in #1454 (I think, I'll update that ref if it's not right, on my phone now and can't confirm -- it's right) and I never found it.

I much, MUCH later realized that was in part due to the Timeline elements all using box-sizing: border-box. Which looks nice, and draws the borders where we want, but it makes all of the math that translates between Timeline positions and JS coordinates come out wrong, because the actual size of everything is slightly smaller than we think it is. There's... a lot of that baked into the code.

It's all written the way it is because every movement the user makes on the Timeline has to get funneled back to the Qt side of things immediately so it can be reacted to on that end. Normally the usage pattern would be that JQuery handles things like resizes all on its own, and just sends the final result off to Qt at the end of the operation. That would make some of the things OpenShot currently does not work the same way, or not be as immediate, but OTOH they might actually work correctly, so...

@jonoomph
Copy link
Member

LGTM! In the future, I want to remove the Angular JS, HTML, webview, and move towards a Qt approach, for example, a few custom QWidgets where we draw the timeline, ruler, tracks, and clips using Qt, still using style sheets (Qt-style), but handle everything on the PyQt side... no more web stuff. That would reduce the size of our installers by a bunch, and reduce the complexity of our dependencies a bunch as well.

@jonoomph jonoomph merged commit 555d583 into OpenShot:develop Jan 27, 2021
@ferdnyc ferdnyc deleted the wider-keyframe-marks branch January 27, 2021 20:40
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jan 27, 2021

In the future, I want to remove the Angular JS, HTML, webview, and move towards a Qt approach, for example, a few custom QWidgets where we draw the timeline, ruler, tracks, and clips using Qt, still using style sheets (Qt-style), but handle everything on the PyQt side... no more web stuff.

@jonoomph I have thoughts that could fill several pages on that, but in general — AngularJS absolutely has to go (like I said, EOL within the year), no question about that. The Modern Qt™ way to implement this part (or any part) of the UI would, I gather, involve implementing it in QML. Which would allow us to continue using JavaScript for the parts of the UI that JavaScript is far better suited for than QtWidgets (like a large scrolling canvas), but without the disconnect from the rest of Qt.

The main problem with the current UI — well, other than the dependency nightmare, which I 100% concur is a real problem, especially when you look at the fact that Qt 6 released without QtWebEngine — but putting aside the packaging details, the other biggest problem is the complete lack of Model/View separation in the Timeline implementation.

I've mentioned before the need for a TimelineModel, which is where the details about placement of things like Layers, Clips, and Transitions should live — with the actual Timeline being just a View on that Model data. (With no knowledge of things like Start and End frames, etc. — as far as the Timeline is concerned, a Clip is a rectangle with a width and a height, placed at a left X coordinate on the canvas... translating from project data elements like Start, End, and Duration to those display-coordinate values is the job of the TimelineModel.)

That'd alleviate the need to duplicate all of the project data into the Timeline, for starters. (No matter how it's implemented. This is all equally applicable to QML, QtWidgets, or any other UI implementation — even the current AngularJS one.) If done right, the end result should be that OpenShot could be run without any Timeline display at all (for things like unit tests), because the TimelineView code would no longer be intrinsic to — or have any involvement with — processing the project data.

(Along similar lines, I'd like to at some point reimplement Export so that there's complete — or any, really — separation between functionality and UI, so that an Export job can be run in the background without having to show the Dialog. Ideally, without requiring any visible UI at all, if necessary. That would allow us to build features like a command-line export tool that bypasses all of the GUI overhead.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants