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

fix: correctly render the tooltip triangle #4560

Merged
merged 12 commits into from
Nov 5, 2024

Conversation

EdsterG
Copy link
Contributor

@EdsterG EdsterG commented Nov 2, 2024

Description

Fixes tooltip triangle rendering.

Before this PR:
before

After this PR:
after

Type of change

  • Bug fix (non-breaking change which fixes an issue)

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.

@EdsterG EdsterG mentioned this pull request Nov 2, 2024
5 tasks
@ffreyer
Copy link
Collaborator

ffreyer commented Nov 4, 2024

From #4451

WGLMakie tooltips used to render correctly for me back in v0.21.8, and then broke a with one of the updates. I just pulled in the latest version v0.21.15 and the triangle is still wrong in WGLMakie. Pushed a fix that works for me #4560, but I think it’s more of a hack than an actual fix.

@ffreyer do you recall the commit in which you fixed the tooltip triangle? I wonder if that fix was OS specific for whatever reason.

Answering this here since the other pr isn't really about this.

I don't really like this as a fix. With lines having AA, you either have to let a bit of the (non tooltip) background show or cut off AA. It also likely fails if you change the size of the tooltip.

The right fix for this should be to pin to the render order so that the lines get drawn after the background mesh components. That should already happen here based on the plot order. Since that's not the case I assume this ends up changing the order:

function sortby(x)
robj = x[3]
plot = screen.cache2plot[robj.id]
# TODO, use actual boundingbox
return Makie.zvalue2d(plot)
end
zvals = sortby.(screen.renderlist)
permute!(screen.renderlist, sortperm(zvals))

where
zvalue2d(x)::Float32 = Float32(Makie.translation(x)[][3] + zvalue2d(x.parent))

or rendering is calculating slightly different, system dependent z values. (I.e. float precision issues)
Either way that should be fixable with a translate!(line_plot, 0, 0, 1) like text has.

@EdsterG
Copy link
Contributor Author

EdsterG commented Nov 4, 2024

Ah, thanks for the pointer. Found where the large z translation was coming from. At the time of writing o[3] has a value of 9000 for some reason, which is derived from text. I hardcoded the z translation to be 1 instead of o[3].

@ffreyer
Copy link
Collaborator

ffreyer commented Nov 4, 2024

No I think that was correct. In DataInspector the tooltip is moved to large z so that it draws over other stuff. Iirc 9000 is the default for that. It gets applied with a translate!(tooltip, 0, 0, z) from outside which changes tooltip.model which then gets processed in the px_pos calculation. text directly uses that. The rect background mesh and the outline use it indirectly by using text bounding boxes. The triangle background mesh does not, it uses a static z = 0, so it needs to still be translated to match.

If you check boundingbox(tooltip.plots[...]) you should end up with both meshes at the same z and lines and text at that z + 1.

@EdsterG
Copy link
Contributor Author

EdsterG commented Nov 4, 2024

Ok, translated the outline by 1. Without this PR the z values are:

text = 9000
bg_mesh = 9000
tr_mesh = 0
outline = 9000

With this PR, the tooltip seems to be rendered correctly, but the z values are:

text = 9000
bg_mesh = 9000
tr_mesh = 0
outline = 9001

I don't understand why the triangle mesh bb has a z value of 0. I guess I don't really understand what boundingbox is returning. For example, translate!(tp, 0, 0, 1) seems to have no impact on the bounding box of tp and translate!(mp, ...) has no impact on the boundingbox of mp. Very confusing.

@ffreyer
Copy link
Collaborator

ffreyer commented Nov 4, 2024

Seems like boundingbox looks at the model matrix in a different way than the backend. But I think the pr should be fine now.

boundingbox is returning the boundingbox of the string, translated to the text plots position. I switched it to string_boundingbox because I think that boundingbox is more likely to change in the future.

@EdsterG
Copy link
Contributor Author

EdsterG commented Nov 4, 2024

Seems like boundingbox looks at the model matrix in a different way than the backend.

So what are possible steps to unify the two? We can probably squash a good bit of rendering bugs with consistency.

@ffreyer
Copy link
Collaborator

ffreyer commented Nov 4, 2024

I noticed that render order was still a problem. E.g. if you have a tooltip translated to z = 100 and add a mesh at z = 50, the outline with AA with the added mesh instead of the tooltip background:

Screenshot 2024-11-04 231310

This happens because only translate/model is considered in render order and the triangle mesh is at z = 100 while the outline is at z = 0. I moved some code around so that the outline ends up in front of the triangle. I also changed the translation to be smaller so that it's less likely for something to squeeze between the outline and background. Hopefully that now works correctly.

The tooltip refimg now includes a test for z order with an orange mesh going from behind to in front of the tooltip:
image

@EdsterG
Copy link
Contributor Author

EdsterG commented Nov 5, 2024

This is great, thanks for pushing the PR to the finish line!

@ffreyer
Copy link
Collaborator

ffreyer commented Nov 5, 2024

Seems like boundingbox looks at the model matrix in a different way than the backend.

So what are possible steps to unify the two? We can probably squash a good bit of rendering bugs with consistency.

This is ultimately a space problem and I'm still not sure how it should actually work. The original data, pixel, clip, relative space options were all meant to be camera related, i.e. changing whether camera.projectionview, camera.pixel_space, I, relative_projection is applied. That leaves transform_func and model unaccounted for. I think the options now are to either handle those independently or include them as space options.

If you just consider the pipeline to be input_data |> apply_transform_func |> apply_model |> apply_projectionview the latter option seems like the best choice. But transformations are also useful with other (camera) projections. E.g. tooltip uses scale!, translate! and rotate! in pixel space, i.e. wants at least input_data |> apply_model |> apply_pixel_space. So should we then have a apply-both, apply-model, apply-neither option for each of data, pixel, clip and relative? How do we name them to communicate what they do?

@ffreyer ffreyer merged commit 4ff461e into MakieOrg:master Nov 5, 2024
20 of 21 checks passed
@EdsterG EdsterG deleted the eddie/jl_fix_tooltip branch November 5, 2024 20:11
@EdsterG
Copy link
Contributor Author

EdsterG commented Nov 5, 2024

Just to confirm, the pipeline you're describing is within a single scene? apply_transform_func means "transform the input data", e.g. log10, sqrt, etc.? apply_model then means, apply the object transforms?

My general understanding of 3D to 2D pipelines is as follows: There's a world coordinate system, every object has a transform that moves it relative to the world coordinate system (the object transform also defined a coordinate system relative to that object). You then compute the "view transform" of every object, which is just its location relative to the camera. Finally, apply a projection transform to create the 2D image and clip anything outside the view frustum.

It's an elementary understand so if you could help me map the Makie pipeline to the version I understand, that would be helpful. Right now I have the following:

  • input_data |> apply_transform_func is equivalent to "object definition", since apply_transform_func can be a nonlinear transform it must be part of the object definition. The object is defined as a set of point relative to the object coordinate system. The transform (potentially nonlinear) will redefine the location of those points.
  • apply_model is equivalent to "transform the object", aka move the object relative to the world coordinate system.
  • apply_projectionview is "project to create the 2D image".

However, this doesn't seem like the correct mapping because I'm struggling to see a clean split between "object transform" and "projection transform" in the current Makie pipeline. I also don't understand where the "object definition" ends and where "object transform" starts, e.g. the model matrix for most plots (and other objects, sliders, etc.) seems to be an identity matrix, but they're magically in different locations relative to each other.

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

Successfully merging this pull request may close these issues.

2 participants