Skip to content

Conversation

@mzxrules
Copy link
Contributor

@mzxrules mzxrules commented Jun 4, 2025

No description provided.

@fig02
Copy link
Collaborator

fig02 commented Jun 4, 2025

forgot your license comment here

static Linef planeIntersectSeg;

Vec3f sp34; // unused
Vec3f _point; // discarded
Copy link
Collaborator

Choose a reason for hiding this comment

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

we dont do leading underscore for "discarded" arguments anywhere else. it goes against our convention, I dont want to introduce it

Copy link
Contributor

Choose a reason for hiding this comment

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

also maybe instead of closestPoint and point it should be planePoint and linePoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cadmic can you or someone else explain your reasoning to me? I'm trying to wrap my head around the bigger picture what these two functions are doing because it could help name two functions in bgcheck:

Math3D_PlaneVsPlaneVsLineClosestPoint
Math3D_PlaneVsLineSegClosestPoint

Like to me, it seems like Math3D_PlaneVsLineSegClosestPoint should be named PlaneVsPlane as well.

Copy link
Contributor

@cadmic cadmic Jun 8, 2025

Choose a reason for hiding this comment

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

The planePoint/linePoint comment was solely based on reading // closestPoint is a point on planeIntersect, sp34 is a point on linePointA, linePointB

But, looking at it harder, I think the comment on Math3D_PlaneVsPlaneVsLineClosestPoint is accurate, but Math3D_PlaneVsLineSegClosestPoint is actually comparing with a "line", not a "line segment". I'd suggest something like the following names:

Math3D_PlaneVsPlaneNewLine -> Math3D_PlaneIntersect
Math3D_PlaneVsPlaneVsLineClosestPoint -> Math3D_PlaneIntersectVsPoint
Math3D_PlaneVsLineSegClosestPoint -> Math3D_PlaneIntersectVsLine

Here PlaneIntersect means the line formed by the intersection of two planes. I didn't want to use Vs because I think that made Math3D_PlaneIntersectVsPoint and Math3D_PlaneIntersectVsLine too confusing

Copy link
Contributor

@cadmic cadmic Jun 8, 2025

Choose a reason for hiding this comment

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

I guess if we want to be fully consistent about removing "ClosestPoint", there should be these renames too:

Math3D_LineClosestToPoint -> Math3D_LineVsPoint
Math3D_LineVsLineClosestTwoPoints -> Math3D_LineVsLine

the idea being that "vs" means any comparison, not necessarily and intersection test

static Linef planeIntersectSeg;

Vec3f sp34; // unused
Vec3f _point; // discarded
Copy link
Contributor

Choose a reason for hiding this comment

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

also maybe instead of closestPoint and point it should be planePoint and linePoint?

@Dragorn421 Dragorn421 added the Waiting for author There are requested changes that have not been addressed label Jun 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Waiting for author There are requested changes that have not been addressed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants