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

Improve Axis3 interactivity performance #1835

Merged
merged 14 commits into from May 12, 2022
Merged

Improve Axis3 interactivity performance #1835

merged 14 commits into from May 12, 2022

Conversation

ffreyer
Copy link
Collaborator

@ffreyer ffreyer commented Apr 23, 2022

Description

Change in text

I noticed that Axis3 can be rather sluggish at times and one reason for it seem to be text updates. A large part of the time spent on updating the angles in Axis3 was in the update of camera.projectionview. From there I intercepted the updates with timed updates with this:

using GLMakie, LaTeXStrings
fig = Figure()
ax = Axis3(fig[1, 1], xlabel = L"Test $\frac{1}{1+x}$")

function print_time(f, i)
    x -> begin
        t = time()
        val = f(x)
        dt = time() - t
        if dt > 1e-3
            printstyled("\nTime: $dt\n", color = :red)
            println("@ index $i")
            flush(stdout)
        end
        return val
    end
end

src = ax.scene.camera.projectionview
for i in eachindex(src.listeners)
    src.listeners[i] = print_time(src.listeners[i], i)
end

fig

and found that most of the time was spend in add_ticks_and_ticklabels!, in (and after) label_pos_rot_valign. (You can find that out by doing autocompletes on the functions held by the observable.)

Well, long story shot - I removed the position trigger from text layouting and that reduced this particular timing from ~0.06s to ~0.006s. I also changed the method with (string, pos) pairs to only update when necessary to avoid rebuilding glyphcollections too much.

In Axis3

For this I tested with src = ax.azimuth. After the text changes the first 3 listeners took ~0.006, ~0.006 and ~0.012s. The first two are mi1 and mi2 and the last is the calculation of projectionview. I changed mi1, mi2 and mi3 to only update on changes which practically eliminates the first two calls.

I also split up some observables for the axis label to avoid triggering text layouting unnecessarily. That (or the changes text! for latexstrings) reduced the last timing to ~0.006s for me.

Type of change

  • Performance

@ffreyer ffreyer changed the title skip unnecessary text updates Improve Axis3 interactivity performance Apr 23, 2022
@@ -298,7 +298,7 @@ end

Makie.step!(st)
## change lengths
textnode.val = push!(textnode[], L"\int_0^5x^2+2ab")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With the changes in the pr this doesn't work anymore for CairoMakie, but updating the nodes in either order normally works

Copy link
Member

Choose a reason for hiding this comment

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

what does that mean?

Copy link
Collaborator Author

@ffreyer ffreyer May 2, 2022

Choose a reason for hiding this comment

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

I think I made the textnode.val = ... -> textnode[] = ... change explicitly for CairoMakie tests to pass

Copy link
Member

Choose a reason for hiding this comment

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

ah that way around...

Copy link
Member

@SimonDanisch SimonDanisch left a comment

Choose a reason for hiding this comment

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

As far as I can tell, this works pretty well :) tested a few latex + text updates and Axis3 interaction

@ffreyer
Copy link
Collaborator Author

ffreyer commented May 4, 2022

It would probably be worth testing updates to attributes like xticklabelalign as well. I think those are the most likely to stop working from this

Copy link
Collaborator

@jkrumbiegel jkrumbiegel left a comment

Choose a reason for hiding this comment

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

Looks good to me although I didn't have time to run it.

@SimonDanisch
Copy link
Member

Hm,

f, ax, pl = scatter(1:4; axis=(xticklabelalign=(:left, :bottom),))
ax.xticklabelalign = (:right, :top)

Doesn't update, but also doesn't update on last tagged version..Also, the block refactor actually broke setting it completely, but I'll make a PR to fix that.

@ffreyer
Copy link
Collaborator Author

ffreyer commented May 9, 2022

https://github.com/JuliaPlots/Makie.jl/blob/b3b0c6daaff5a7c5d5ed36310e5a65fc11832b67/GLMakie/src/drawing_primitives.jl#L313-L327

This is what makes updates of textsize, align, font and probably everything else that isn't position, offset or transfunc fail with Vector{GlyphCollection}. Why is it there?

@ffreyer
Copy link
Collaborator Author

ffreyer commented May 9, 2022

Good old synchronization issues is why it's there...

@ffreyer
Copy link
Collaborator Author

ffreyer commented May 10, 2022

Didn't mean to link the second issue here lol. If tests pass the issue with updates to text attributes not propagating properly with vector positions should be fixed.

@ffreyer
Copy link
Collaborator Author

ffreyer commented May 10, 2022

Somehow this changed the colors in "Dynamically adjusting number of particles in a meshscatter"... It looks fine though. Should we merge this with the attribute update fixes anyway?

Screenshot from 2022-05-10 20-27-19

@SimonDanisch
Copy link
Member

Somehow this changed the colors in "Dynamically adjusting number of particles in a meshscatter".

ColorTypes.jl changed their rand implementation, which lead to this... I rebased against master and updated the ref images, so hopefully should turn green now!

@ffreyer ffreyer merged commit 4893982 into master May 12, 2022
@ffreyer ffreyer deleted the ff/perf branch May 12, 2022 08:19
kunzaatko pushed a commit to kunzaatko/Makie.jl that referenced this pull request May 16, 2022
* skip unnecessary updates

* use input observable

* reduce update dublication

* fix test

* fix tests

* fix test

* undo some bad changes

* hacky fix for GLMakie text updates

* undo last change

* hacky fix for text updates MakieOrg#2

* fix typos

Co-authored-by: SimonDanisch <sdanisch@gmail.com>
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

3 participants