JuliaGeometry / GeometryBasics.jl

Basic Geometry Types
MIT License
165 stars 54 forks source link

add a docstring for `Rect` and its aliases #208

Closed putianyi889 closed 6 months ago

asinghvi17 commented 6 months ago

Looks good to me!

SimonDanisch commented 6 months ago

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

knuesel commented 6 months ago

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

SimonDanisch commented 6 months ago

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 commented 6 months ago

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

knuesel commented 6 months ago

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:

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

jkrumbiegel commented 6 months ago

Valid points, I agree

ffreyer commented 6 months ago

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 commented 6 months ago

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
SimonDanisch commented 6 months ago

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

knuesel commented 6 months ago

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

SimonDanisch commented 6 months ago

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