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

Add tick event #3948

Draft
wants to merge 36 commits into
base: master
Choose a base branch
from
Draft

Add tick event #3948

wants to merge 36 commits into from

Conversation

ffreyer
Copy link
Collaborator

@ffreyer ffreyer commented Jun 8, 2024

Description

This pr adds events.tick which currently contains:

struct Tick
    state::TickState    # rendering state (paused, skipped, one-time or normal render)
    count::UInt64       # number of frames rendered 
    time::Float64       # since screen/backend event initialization
    delta_time::Float64 # between last and current frame
end

This is more verbose than just a delta time mostly because of how screen.render_tick is used in GLMakie. Updates to it are grouped with event polling since we have mouse position updates depend on it. So it triggers when

  • rendering, regardless of whether a frame is rendered or reused
  • rendering is paused, as not processing events would make the OS assume the program is not responding
  • plot and scene insertion for the same reason (these get filtered out)
  • calls to colorbuffer (i.e. save, record)

Depending on what causes the tick update a different state flag is passed. event_delta_time is always updated based on the time since the last tick. frame_delta_time is always based on the last renderloop event (paused/skipped/drawn) or colorbuffer event (i.e. a draw for save or record) and set to 0 when the current event is not one of those. So it's the time between the last and current frame, even if plot insertions gets between frames.

Currently the pr should be pretty functional, but I think we need to discuss what we want ticks to be on the user side.

  • Should we ignore ticks coming from plot insertions, i.e. only include ticks related to rendering? (Which would make event_delta_time redundant) (Currently does)
  • Should a paused renderloop still tick? (Currently counted)
  • Should we track more data in ticks, like frame count and time-since-start as suggested in Global figure time #3299 (comment) (How should we treat reused frames with frame count? Should record reset the time-since-start?) (Currently tracking frame count, time since start, time since last frame)
  • Should record overwrite the delta times in ticks to match the fps of the recording (rather than using the real time between rendered frames)? (Currently does not. My thought being that you can fairly easily use (tick.count - first_count) / fps to get the relevant delta_time.)

Supersedes #3299
Related: #3163

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • Added an entry in CHANGELOG.md (for new features and breaking changes)
  • Added or changed relevant sections in the documentation
  • Added unit tests for new algorithms, conversion methods, etc.
  • Added reference image tests for new plotting functions, recipes, visual options, etc.

@MakieBot
Copy link
Collaborator

MakieBot commented Jun 8, 2024

Compile Times benchmark

Note, that these numbers may fluctuate on the CI servers, so take them with a grain of salt. All benchmark results are based on the mean time and negative percent mean faster than the base branch. Note, that GLMakie + WGLMakie run on an emulated GPU, so the runtime benchmark is much slower. Results are from running:

using_time = @ctime using Backend
# Compile time
create_time = @ctime fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @ctime Makie.colorbuffer(display(fig))
# Runtime
create_time = @benchmark fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @benchmark Makie.colorbuffer(fig)
using create display create display
GLMakie 4.28s (4.24, 4.35) 0.04+- 106.79ms (103.24, 112.24) 3.62+- 485.91ms (477.41, 495.07) 6.24+- 8.51ms (8.25, 8.76) 0.21+- 25.89ms (25.49, 26.27) 0.25+-
master 4.27s (4.22, 4.34) 0.04+- 107.88ms (103.93, 111.28) 2.60+- 489.64ms (476.98, 512.22) 13.29+- 8.44ms (8.24, 8.87) 0.23+- 25.83ms (25.63, 26.07) 0.13+-
evaluation 1.00x invariant, 0.01s (0.36d, 0.52p, 0.04std) 1.01x invariant, -1.09ms (-0.35d, 0.53p, 3.11std) 1.01x invariant, -3.72ms (-0.36d, 0.52p, 9.77std) 0.99x invariant, 0.07ms (0.33d, 0.55p, 0.22std) 1.00x invariant, 0.07ms (0.32d, 0.56p, 0.19std)
CairoMakie 3.89s (3.82, 3.93) 0.03+- 106.54ms (105.34, 108.60) 1.18+- 131.74ms (129.34, 140.04) 3.79+- 8.35ms (8.19, 8.57) 0.12+- 983.78μs (979.99, 987.70) 3.09+-
master 3.67s (3.56, 3.90) 0.11+- 109.63ms (106.04, 119.62) 4.69+- 134.04ms (130.19, 139.89) 3.74+- 8.33ms (8.18, 8.55) 0.15+- 992.38μs (980.77, 1017.44) 12.22+-
evaluation 0.94x slower❌, 0.22s (2.63d, 0.00p, 0.07std) 1.03x invariant, -3.09ms (-0.90d, 0.14p, 2.93std) 1.02x invariant, -2.29ms (-0.61d, 0.28p, 3.77std) 1.00x invariant, 0.02ms (0.14d, 0.79p, 0.14std) 1.01x invariant, -8.6μs (-0.96d, 0.12p, 7.66std)
WGLMakie 4.58s (4.49, 4.71) 0.07+- 110.08ms (105.41, 117.50) 4.48+- 9.41s (9.18, 9.69) 0.20+- 9.69ms (9.45, 9.90) 0.17+- 71.18ms (70.45, 71.82) 0.59+-
master 4.57s (4.49, 4.71) 0.08+- 108.99ms (106.90, 110.80) 1.62+- 9.36s (9.09, 9.52) 0.16+- 9.85ms (9.28, 11.88) 0.92+- 73.15ms (70.43, 84.98) 5.25+-
evaluation 1.00x invariant, 0.01s (0.14d, 0.80p, 0.07std) 0.99x invariant, 1.09ms (0.32d, 0.56p, 3.05std) 1.00x invariant, 0.05s (0.26d, 0.64p, 0.18std) 1.02x invariant, -0.16ms (-0.25d, 0.66p, 0.55std) 1.03x invariant, -1.97ms (-0.53d, 0.36p, 2.92std)

@jkrumbiegel
Copy link
Member

Closing #3163 would need using this to replace all time-dependent animations in Makie. I think that might just be the toggle switch animation, the textbox cursor animation and the axis reset timer after scrolling.

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jun 10, 2024

Example from #3299 with this:

begin
    fig = Figure()

    axs = [Axis(fig[i, j]) for i in 1:2, j in 1:2]
    xlims = [[-1.2, 0], [0, 1.2]]
    ylims = [[0, 1.2], [-1.2, 0]]
    wavelengths = [
        5 8
        11 14
    ]
    ω = 0.3
    θs = range(0, 2π ; length = 1000)

    for i in 1:2 
        for j in 1:2
            ax = axs[i, j]
            xlims!(ax, xlims[j])
            ylims!(ax, ylims[i])
            w = wavelengths[i, j]

            ρs = @lift [sin(w *+ ω * $(events(fig).tick).count * 0.033)) for θ in θs]
            xx = @lift $ρs .* cos.(θs)
            yy = @lift $ρs .* sin.(θs)

            lines!(ax, xx, yy)
        end
    end
    
    fig
end

record(fig, "flower.mp4", 1:240) do i
end

It's a little awkward that you have to use tick.,count instead of tick.time for record here, since time is always real time atm. I'm still not sure if it should use fos based times in record. That would simplify this example a little more, but you couldn't use ticks for rendering stats during record anymore.

@jkrumbiegel
Copy link
Member

jkrumbiegel commented Jun 11, 2024

tick.time for record here, since time is always real time

Why not both if both can be useful. For the animations in record I would want a frame time and not the time that elapsed in reality.

Edit: Ah I see, the animations would then probably look wrong in live mode. Maybe a keyword to record could control which time base should be used? Sometimes people want to record stuff live I guess but most of the time I'd think not.

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jun 11, 2024

I moved colorbuffer ticks to Makie record/save so they can synchronize to record framerates now. I'm not entirely sure if they are in the correct functions, but the tests I've written are passing at least.

The ticks produced with WGLMakie are a bit of a mess though. On screen initialization there tends to be a burst of them, and during record the normal render loop is also running, producing ticks. Idk if there is anything we can do about that, maybe @SimonDanisch can have a look?

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jun 12, 2024

┌ Error: Cannot resolve @ref for md"[`TickState`](@ref)" in src/api.md.
│ - No docstring found in doc for binding `Makie.TickState`.
│ - No docstring found in doc for binding `Main.TickState`.

Does this not work with enums?

Also "Normals of a Cat" is failing due to changes in axis limits. Not idea where that is coming from. I don't see it locally, not even as a temporary state of the axis

@jkrumbiegel
Copy link
Member

Normals of a cat has been flaky on CI for me too. Haven't yet tried to figure out what the problem is.


For an interactive figure this will produce an animation synchronized with real time. Within `record` the tick times match up the set `framerate` such that the animation in the produced video matches up with real time.

The only exception here is WGLMakie which currently runs the render loop during recording. Because of that `RegularRenderTick` and `OneTimeRenderTick` will mix during `record`, with the former being based on real time and the latter based on video time. If you do not restrict to `OneTimeRenderTick` here the resulting video will run faster than real time with `tick.delta_time` and may jump with `tick.time` and `tick.count`.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could probably avoid this by eating up the regular render ticks during recording

cb = on(events(fig).tick, priority = typemax(Int)) do tick
    return Consume(tick.state != Makie.OneTimeRenderTick)
end
# record ...
off(cb)

but it's not clear to me how to do that safely. My first thought would be to have the creation of VideoStream start this and save end this, but that doesn't seem save to me. I.e. VideoStream being created doesn't necessarily mean only recording takes place and you could save another video after the first save...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added that to record and suggested doing this for the lower level functions in the docs for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok I changed my mind and just attached the filtering to VideoStream and save/a finalizer. Mostly because otherwise the tick event order is a bit weird.

In the previous version the tick updates triggered just before colorbuffer(). So the order of operations was:

# record(foo, ...)
foo()
tick_update()
colorbuffer()

You'd still get updates coming from on(..., tick) before the render, but you couldn't use events(fig).tick to get the current frames time or count in foo(). I change things around a bit so that record effectively becomes:

tick_update()
foo()
colorbuffer()

so you can. This makes the filtering more important though, because the update for tick N actually happens at the end of frame N-1 in the recordframe!() call. (The first tick happens on VideoStream creation)

I also adjusted ticks so that the first tick is at count and time = 0.

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jun 19, 2024

With the current state of this pr you can set up an animation like this for example:

angle = Observable(0.0)
pos = map(angle) do phi0
    [Point2f(cos(phi + phi0), sin(phi + phi0)) for phi in range(0, 2pi, 101)[1:end-1]]
end
f, a, p = scatter(pos, color = 1:100, strokewidth = 1, strokecolor = :black)
scatter!(pos[][1], marker = Circle, markersize = 12, color = :transparent, strokewidth = 2, strokecolor = :red)
on(events(f).tick) do tick
    angle[] = 0.5 * pi * tick.time
end
f

and you will see 1 revolution every 4s independently of (set and real) frame rate.

You can record this animation by just calling record:

record(f, "test.mp4", 1:120, framerate = 30) do _
    nothing
end

This will generate a new set of ticks starting at count = t = 0 with time steps of 1.0 / framerate = 0.0333.... So it will reset the animation to the time = 0 state, and then record one revolution, as 120 frames / 30fps = 4s.

If you then redisplay the figure, the interactive ticks will again be based on the renderloop. The first tick may have a long delta_time = current_time - time_before_record.

const time_per_frame = (1 / fps) * 1000; // default is 30 fps
// make sure we immediately render the first frame and dont wait 30ms
let last_time_stamp = performance.now();
function renderloop(timestamp) {
if (timestamp - last_time_stamp > time_per_frame) {
const canvas = renderer.domElement;
canvas.dispatchEvent( new Event('render') );
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we should spam the websocket with a tick for every frame...
I know that this is the correct way, but maybe we need an approximation for WGLMakie for now?
We should definitely benchmark this, since an additional 30 messages per seconds could likely make message congestion worse.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved it to the Julia side with a Timer. If I understand correctly these events wouldn't be synchronized well with rendering due to message passing anyway. The problem now is that Timer is just as bad as sleep in terms of accuracy, i.e. it's running at 20-25 ticks per second when targeting 30.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking of generalizing #3954, merging it into this and using the same approach for WGLMakie ticks

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.

None yet

4 participants