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

Allow higher dimensional geometries in poly #4738

Merged
merged 4 commits into from
Feb 18, 2025
Merged

Allow higher dimensional geometries in poly #4738

merged 4 commits into from
Feb 18, 2025

Conversation

ffreyer
Copy link
Collaborator

@ffreyer ffreyer commented Jan 18, 2025

Description

This reverts a change from the breaking release, allowing something like poly(Rect3f[...]) again. (Do we want to allow poly to draw 3D shapes?)

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.

@MakieBot
Copy link
Collaborator

MakieBot commented Jan 18, 2025

Benchmark Results

SHA: 34bf45680982a5018d8a299ff525a4c51a622d5e

Warning

These results are subject to substantial noise because GitHub's CI runs on shared machines that are not ideally suited for benchmarking.

GLMakie
CairoMakie
WGLMakie

@jkrumbiegel
Copy link
Member

I personally don't think it makes sense to have 3d polys. The only one that makes sense is a 2d poly positioned in some plane in 3D space. But I would only add a nicer interface to define that rather than actually accepting 3D points.

@SimonDanisch
Copy link
Member

Hm, the problem is, we made poly in a way, that it's just a mesh + wireframe, so it has been used for that...
So, I'd say we bring it back for now and take some time before we change the behavior of poly?
I feel like mesh + wireframe is a reasonable functionality we should have, the question is, if poly is the right place.
Due to just calling it poly it may also refer to polytope, which fits relatively well of what it actually currently visualizes.

@jkrumbiegel
Copy link
Member

I would say the mesh + wireframe is just an implementation detail and in cairomakie it's not accurate either.

@SimonDanisch
Copy link
Member

I would say the mesh + wireframe is just an implementation detail and in cairomakie it's not accurate either.

I meant, poly currently offers to draw a mesh + outline, which users rely on and use and which is something that's nice to have.
So I didn't mean the implementation detail ;)

@ffreyer ffreyer merged commit f0bb495 into master Feb 18, 2025
22 checks passed
@ffreyer ffreyer deleted the ff/fix-poly branch February 18, 2025 14:02
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.

4 participants