-
-
Notifications
You must be signed in to change notification settings - Fork 306
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
Small clean ups #1717
Small clean ups #1717
Conversation
src/camera/projection_math.jl
Outdated
function space_to_clip_obs(cam::Camera, space::Symbol, projectionview::Bool) | ||
if is_data_space(space) | ||
cam.projectionview[] | ||
return projectionview ? cam.projectionview : cam.projection | ||
elseif is_pixel_space(space) | ||
cam.pixel_space[] | ||
return cam.pixel_space | ||
elseif is_relative_space(space) | ||
Mat4f(2, 0, 0, 0, 0, 2, 0, 0, 0, 0, 1, 0, -1, -1, 0, 1) | ||
return Mat4f(2, 0, 0, 0, 0, 2, 0, 0, 0, 0, 1, 0, -1, -1, 0, 1) | ||
elseif is_clip_space(space) | ||
Mat4f(I) | ||
return Mat4f(I) | ||
else | ||
error("Space $space not recognized. Must be one of $(spaces())") | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relative and clip space should also return observables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it seems the observable version isn't needed... I'll remove it
GLMakie/src/drawing_primitives.jl
Outdated
get!(gl_attributes, :projection) do | ||
Makie.space_to_clip_obs(cam, space[], false) | ||
return map(cam.projection, space) do _, space | ||
return Makie.space_to_clip(cam, space, true) | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The matrix here can be cam.projection
or cam.pixel_space
so it should trigger on both of those and space
. Also seems like you didn't update this correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for 2d, 3d and pixel cameras reacting to pixelspace
isn't necessary but for RelativeCamera
it would be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, for simplicity I'll just add pixel_space to it for now...
The right way to do this is hard to express with observables... I need to think about a cleaner way going forward...
* use markerspace::Symbol for text and scatter * adjust camera matrices based on space * only include data space plots in limits * add is_data_space etc * update text * cleanup markerspace * markerspace for scatter * fix interactivity * add default marker/-space * add efault space * get WGLMakie working * fix heatmap * fix tests * fix typo * get CairoMakie working * fix circle marker transform * add 2D space+markerspace tests * fix mesh3D space * add 3D space test * fix poly and band * change space to markerspace * fix rotations * fix 1.3 test error? * fix 1.3 * fix 1.3 error * add space attribute * cleanup comments * add news entry * add documentation for space * add tests for space * fix typo * move preprojection into shaders * minor cleanup * remove deleted import * fix test * clarify comment * Small clean ups (#1717) * remove overly eager optimizations (#1713) * small clean ups * clean up implementation a bit, use only one name for space * address review * fix usage of space_to_clip * fix cairomakie * fix fastpixel and add tests Co-authored-by: Simon <sdanisch@protonmail.com>
Will add some more clean ups tomorrow