Skip to content

Line Render order & orthographic camera min/max fixes #384

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.

Sign up for GitHub

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

Merged
AntonJames-Sistence merged 6 commits into from
Sep 25, 2025
Merged

Line Render order & orthographic camera min/max fixes #384

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
69ba4be
add render order fix
AntonJames-Sistence Sep 24, 2025
b1732ac
add orthographic camera min/max distance controll
AntonJames-Sistence Sep 24, 2025
50ac207
minor render order adjustment
AntonJames-Sistence Sep 25, 2025
4340229
bug fix
AntonJames-Sistence Sep 25, 2025
53e304c
combine values into isActiveState; add min/max zoom props for ortho c…
AntonJames-Sistence Sep 25, 2025
ab85078
remove distance from the useEffect dep
AntonJames-Sistence Sep 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/CameraControls/CameraControls.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,18 @@ export const CameraControls: FC<
? ThreeCameraControls.ACTION.ZOOM
: ThreeCameraControls.ACTION.DOLLY;
}
}, [disabled, mode]);

// For orthographic cameras, use distance parameters as zoom limits
if (isOrthographic && cameraRef.current) {
// Convert distance to zoom values (inverse relationship)
cameraRef.current.maxZoom = maxDistance
? 50000 / minDistance
Copy link
Contributor

@SerhiiTsybulskyi SerhiiTsybulskyi Sep 25, 2025

Choose a reason for hiding this comment

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

@AntonJames-Sistence Should we allow modification of this constant? Maybe better to add this as props with a default value to have the ability to change it if needed?

Copy link
Contributor Author

@AntonJames-Sistence AntonJames-Sistence Sep 25, 2025

Choose a reason for hiding this comment

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

@SerhiiTsybulskyi we can modify maxDistance/minDistance

Copy link
Contributor Author

@AntonJames-Sistence AntonJames-Sistence Sep 25, 2025

Choose a reason for hiding this comment

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

ok I think I need a better idea for this min/max zoom, because it actually doesn't work as expected

Copy link
Contributor Author

@AntonJames-Sistence AntonJames-Sistence Sep 25, 2025

Choose a reason for hiding this comment

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

@SerhiiTsybulskyi I had to add min/max zoom props, since there's no actual correlation between zoom and dolly, as I thought in the beginning

: undefined;
cameraRef.current.minZoom = minDistance
? 50000 / maxDistance
Copy link

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

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

The zoom limit calculation uses minDistance in the denominator when maxDistance exists, but this should use minDistance for maxZoom calculation. The logic appears reversed - maxZoom should be calculated from maxDistance, not minDistance.

Suggested change
? 50000 / minDistance
: undefined;
cameraRef.current.minZoom = minDistance
? 50000 / maxDistance
? 50000 / maxDistance
: undefined;
cameraRef.current.minZoom = minDistance
? 50000 / minDistance

Copilot uses AI. Check for mistakes.
: undefined;
Copy link

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

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

The magic number 50000 is used for zoom calculations without explanation. Consider extracting this as a named constant with a descriptive comment explaining its purpose in the distance-to-zoom conversion formula.

Copilot uses AI. Check for mistakes.
}
}, [disabled, mode, minDistance, maxDistance]);

useEffect(() => {
const onControl = () => setPanning(true);
Expand Down
3 changes: 2 additions & 1 deletion src/symbols/Edge.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,7 @@ export const Edge: FC<EdgeProps> = ({
);

return (
<group position={[0, 0, active ? 1 : 0]}>
<group position={[0, 0, active || isActive || isSelected ? 1 : 0]}>
{isSelfLoop && selfLoopCurve ? (
<SelfLoop
id={id}
Expand Down Expand Up @@ -528,6 +528,7 @@ export const Edge: FC<EdgeProps> = ({
id={id}
opacity={selectionOpacity}
size={size}
renderOrder={isActive || active || isSelected ? 0 : -1}
onClick={event => {
if (!disabled) {
onClick?.(edge, event);
Expand Down
8 changes: 7 additions & 1 deletion src/symbols/Line.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ export interface LineProps {
*/
size?: number;

/**
* The render order of the line. Useful when edges are rendered on top of each other.
*/
renderOrder?: number;

/**
* A function that is called when the line is clicked.
*/
Expand Down Expand Up @@ -124,6 +129,7 @@ export const Line: FC<LineProps> = ({
id,
opacity = 1,
size = 1,
renderOrder = -1,
onContextMenu,
onClick,
onPointerOver,
Expand Down Expand Up @@ -212,7 +218,7 @@ export const Line: FC<LineProps> = ({
return (
<mesh
userData={{ id, type: 'edge' }}
renderOrder={-1}
renderOrder={renderOrder}
onPointerOver={onPointerOver}
onPointerOut={onPointerOut}
onClick={onClick}
Expand Down
Loading