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 a docstring for Rect and its aliases #208

Merged
merged 8 commits into from
Feb 8, 2024

Conversation

putianyi889
Copy link
Contributor

No description provided.

@asinghvi17
Copy link
Contributor

Looks good to me!

@SimonDanisch
Copy link
Member

Thank you! I'm thinking to add a float64 alias here as well here.
How should we call it? Rectf64?
@jkrumbiegel, @ffreyer

@knuesel
Copy link
Contributor

knuesel commented Feb 7, 2024

Rectd would be nice and consistent with OpenCV, Rhino, Java vecmath and Eigen (spirit of #61)...

@SimonDanisch
Copy link
Member

Yeah, that was my favorite before people didn't want it for Point... Its a bit confusing because of d-> dim, but if people agree on it, I'd be up for it.

@jkrumbiegel
Copy link

Maybe Rectd2 is better than Rect2d? But then we'd have to change all the Point2fs to Pointf2s as well

src/primitives/rectangles.jl Outdated Show resolved Hide resolved
src/primitives/rectangles.jl Outdated Show resolved Hide resolved
Co-authored-by: Jeremie Knuesel <knuesel@gmail.com>
@knuesel
Copy link
Contributor

knuesel commented Feb 7, 2024

Maybe Rectd2 is better than Rect2d? But then we'd have to change all the Point2fs to Pointf2s as well

Is the concern that "Rect2d" reads like "2 dimensional rect"? I thought the same but I found it quite easy to get used to Point2d, Vec2d, Rect2d. Some things to consider:

  • The 2,d sequence in Rect2d corresponds to the sequence of type parameters: it's Rect{N,T} not Rect{T,N}. It's also natural to have the "smallest information" come last, i.e. Float32 vs Float64 is a smaller change than 2D vs 3D.
  • I find easier to read Rect2f, Rect2d, Vec2d, than Rectf2, Rectd2, Vecd2. The digit gives a nice visual separation between the word and the f or d suffix.

I suspect these two points are why most APIs use Rect2d rather than Rectd2.

@jkrumbiegel
Copy link

Valid points, I agree

@ffreyer
Copy link
Collaborator

ffreyer commented Feb 7, 2024

I think I like Rect2d more than Rect2f64 and Rectd2. Looks cleaner and is easier to type. The lower case d also kind of matches glsl/opengl and glm's dvec types.
If we want to also use d for dimensions we could enforce that D to be upper case. "double" vs "Dimension". Or just write out "dim/Dim".

@jkrumbiegel
Copy link

jkrumbiegel commented Feb 7, 2024

Is there stuff exported from Makie where the suffix 2d is currently used to mean two-dimensional?

Edit:

Not much

julia> filter(x -> endswith(string(x), r"2[dD]"), names(CairoMakie))
2-element Vector{Symbol}:
 :Camera2D
 :cam2d

Copy link
Contributor

@knuesel knuesel left a comment

Choose a reason for hiding this comment

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

Just one thing to clarify for this PR: do we want to document RectT, or is it rather meant as internal? It's not exported and I don't think I've seen it in user code so far.

@SimonDanisch
Copy link
Member

I think it's internal, but we could still mention it shortly?
But I wouldn't want to export it.

Co-authored-by: Jeremie Knuesel <knuesel@gmail.com>
Co-authored-by: Jeremie Knuesel <knuesel@gmail.com>
@knuesel
Copy link
Contributor

knuesel commented Feb 8, 2024

I guess this doc PR can be merged and new aliases can be introduced in a new PR if desired?

@SimonDanisch
Copy link
Member

Yeah I was thinking about it and i was leaning towards merging it now :)

@SimonDanisch SimonDanisch merged commit 4885d07 into JuliaGeometry:master Feb 8, 2024
9 checks passed
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.

6 participants