feat: make arrows colorful and metadata #430
Conversation
Co-authored-by: vital2580 <[email protected]>
|
Cursor Agent can help with this pull request. Just |
WalkthroughAdds optional metadata propagation for waypoint creation via the custom channel, extends sprite creation to accept metadata, introduces onlyLeftRight clamping for offscreen arrows, changes arrow visuals (size, stroke, shape, fill derived from waypoint color), and sets arrow sprite renderOrder/initial visibility. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant S as Game Server
participant C as Client Channel Handler
participant R as Renderer (Waypoints)
participant F as Sprite Factory
rect rgba(230,240,255,0.5)
Note over S,C: minecraft-web-client:waypoint-add
S->>C: Packet { id, position, color, metadataJson? }
C->>C: Parse metadataJson (try/catch)
C->>R: addWaypoint({ id, position, color, metadata })
R->>F: createWaypointSprite({ color, label, metadata })
F-->>R: Waypoint sprite (arrow styled by color)
end
rect rgba(240,230,255,0.5)
Note over R,F: Offscreen indicator update
R->>F: updateOffscreenArrow(...)
alt metadata.onlyLeftRight == true
F->>F: Clamp direction to horizontal/vertical edge (onlyLeftRight)
else Default behavior
F->>F: Use full directional vector for placement
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
renderer/viewer/three/waypoints.ts (2)
16-21: Type metadata explicitly (avoid any).Define a narrow type (e.g., WaypointMetadata { onlyLeftRight?: boolean }) and use it here to prevent shape drift.
Apply this diff:
interface WaypointOptions { color?: number label?: string minDistance?: number - metadata?: any + metadata?: { onlyLeftRight?: boolean } }
78-83: Align initial sprite position with stored (+0.5) coordinates.Prevents a first-frame “jump” when the update loop runs.
Apply this diff:
const sprite = createWaypointSprite({ - position: new THREE.Vector3(x, y, z), + position: new THREE.Vector3(x + 0.5, y + 0.5, z + 0.5), color, label: (label || id), metadata, })src/customChannels.ts (2)
94-103: Harden JSON parsing and shape.If metadataJson parses to a non-object (string/array), coerce to {} to avoid surprises downstream.
Apply this diff:
let metadata: any = {} if (data.metadataJson && data.metadataJson.trim() !== '') { try { - metadata = JSON.parse(data.metadataJson) + const parsed = JSON.parse(data.metadataJson) + metadata = parsed && typeof parsed === 'object' && !Array.isArray(parsed) ? parsed : {} } catch (error) { console.warn('Failed to parse waypoint metadataJson:', error) } }
104-109: Ensure addWaypoint options include typed metadata.onlyLeftRight
WaypointOptions.metadata is currently typed asany— change it tometadata?: { onlyLeftRight?: boolean }(also updatecreateWaypointSprite's options) soThreeJsBackendMethods.addWaypoint(ReturnType ofgetBackendMethodsin renderer/viewer/three/graphicsBackend.ts) exposes the correct type.
renderer/viewer/three/waypoints.ts; renderer/viewer/three/waypointSprite.ts.renderer/viewer/three/waypointSprite.ts (4)
53-54: Type metadata explicitly.Prefer a concrete shape to avoid accidental overreach.
Apply this diff:
- metadata?: any, + metadata?: { onlyLeftRight?: boolean },Optionally add a shared type alias if reused.
135-151: Fix trailing whitespace (CI failing).CI reports “Trailing spaces not allowed” at Lines 135, 142, 150. Trim blank-line spaces.
Apply this diff:
- + @@ - + @@ - +
179-181: onlyLeftRight behavior: tie-breaker refinement.When rx ≈ 0 and vertical is dominant, code always selects right. Consider using the world-space x sign (or last arrow side) to avoid bias/jitter.
Example tweak:
// inside the onlyLeftRight block if (Math.abs(rx) > Math.abs(ry)) { ry = 0 } else { // use world-space x sign as tie-breaker when rx ~ 0 const sign = Math.sign(x) || 1 rx = sign > 0 ? 1 : -1 ry = 0 }Also applies to: 226-238
299-308: Remove arrow sprite from scene on dispose.Dispose frees material but leaves the sprite in the scene graph.
Add:
if (arrowSprite?.parent) { arrowSprite.parent.remove(arrowSprite) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
renderer/viewer/three/waypointSprite.ts(5 hunks)renderer/viewer/three/waypoints.ts(2 hunks)src/customChannels.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/vars-usage.mdc)
src/**/*.ts: You may use the global variablebotdirectly in any file under thesrc/directory (e.g.,src/mineflayer/playerState.ts).
Insrc/code, you may use the global variableappViewerfromsrc/appViewer.tsdirectly. Do not importappVieweror usewindow.appViewer; use the globalappViewervariable as-is.
Files:
src/customChannels.ts
renderer/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/vars-usage.mdc)
renderer/**/*.ts: Do not use the global variablebotdirectly in any file under therenderer/directory or its subfolders (e.g.,renderer/viewer/three/worldrendererThree.ts).
In renderer code, all bot/player state and events must be accessed via explicit interfaces, state managers, or passed-in objects, never by referencingbotdirectly.
In renderer code (such as inWorldRendererThree), use theplayerStateproperty (e.g.,worldRenderer.playerState.gameMode) to access player state. The implementation forplayerStatelives insrc/mineflayer/playerState.ts.
Files:
renderer/viewer/three/waypoints.tsrenderer/viewer/three/waypointSprite.ts
🧬 Code graph analysis (2)
src/customChannels.ts (1)
renderer/viewer/three/threeJsMethods.ts (1)
getThreeJsRendererMethods(4-15)
renderer/viewer/three/waypoints.ts (1)
renderer/viewer/three/waypointSprite.ts (1)
createWaypointSprite(46-323)
🪛 GitHub Check: build-and-deploy
renderer/viewer/three/waypointSprite.ts
[failure] 150-150:
Trailing spaces not allowed
[failure] 142-142:
Trailing spaces not allowed
[failure] 135-135:
Trailing spaces not allowed
🪛 GitHub Actions: CI
renderer/viewer/three/waypointSprite.ts
[error] 135-135: Trailing spaces not allowed. (stylistic/no-trailing-spaces)
🔇 Additional comments (4)
renderer/viewer/three/waypoints.ts (1)
75-76: LGTM: options destructuring and propagation.Metadata is forwarded correctly to the sprite factory.
src/customChannels.ts (1)
85-89: LGTM: channel schema extended with metadataJson.Optional JSON payload is added without breaking existing fields.
renderer/viewer/three/waypointSprite.ts (2)
19-21: LGTM: larger, padded arrows improve visibility.Increased pixelSize and padding are reasonable defaults.
138-149: LGTM: arrow shape and colorization.Arrow uses waypoint color with black stroke; renderOrder set higher than label sprite.
|
|
||
| // Draw arrow shape | ||
| ctx.beginPath() | ||
| ctx.moveTo(size * 0.2, size * 0.5) | ||
| ctx.lineTo(size * 0.8, size * 0.5) | ||
| ctx.lineTo(size * 0.5, size * 0.2) | ||
| ctx.moveTo(size * 0.15, size * 0.5) | ||
| ctx.lineTo(size * 0.85, size * 0.5) | ||
| ctx.lineTo(size * 0.5, size * 0.15) | ||
| ctx.closePath() | ||
| ctx.lineWidth = 4 | ||
|
|
||
| // Use waypoint color for arrow | ||
| const colorHex = `#${color.toString(16).padStart(6, '0')}` | ||
| ctx.lineWidth = 6 | ||
| ctx.strokeStyle = 'black' | ||
| ctx.stroke() | ||
| ctx.fillStyle = 'white' | ||
| ctx.fillStyle = colorHex | ||
| ctx.fill() | ||
|
|
||
| const texture = new THREE.CanvasTexture(canvas) |
There was a problem hiding this comment.
Keep arrow color in sync with setColor().
Arrow texture is baked once with the initial color and won’t update after setWaypointColor. Update the arrow texture on color change.
Apply this diff within ensureArrow to allow reuse of the texture generator:
function ensureArrow () {
if (arrowSprite) return
- const size = 128
- const canvas = document.createElement('canvas')
- canvas.width = size
- canvas.height = size
- const ctx = canvas.getContext('2d')!
- ctx.clearRect(0, 0, size, size)
-
- // Draw arrow shape
- ctx.beginPath()
- ctx.moveTo(size * 0.15, size * 0.5)
- ctx.lineTo(size * 0.85, size * 0.5)
- ctx.lineTo(size * 0.5, size * 0.15)
- ctx.closePath()
-
- // Use waypoint color for arrow
- const colorHex = `#${color.toString(16).padStart(6, '0')}`
- ctx.lineWidth = 6
- ctx.strokeStyle = 'black'
- ctx.stroke()
- ctx.fillStyle = colorHex
- ctx.fill()
-
- const texture = new THREE.CanvasTexture(canvas)
+ const texture = makeArrowTexture(color)
const material = new THREE.SpriteMaterial({ map: texture, transparent: true, depthTest: false, depthWrite: false })
arrowSprite = new THREE.Sprite(material)
arrowSprite.renderOrder = 12
arrowSprite.visible = false
if (arrowParent) arrowParent.add(arrowSprite)
}
+
+function makeArrowTexture (c: number): THREE.CanvasTexture {
+ const size = 128
+ const canvas = document.createElement('canvas')
+ canvas.width = size
+ canvas.height = size
+ const ctx = canvas.getContext('2d')!
+ ctx.clearRect(0, 0, size, size)
+ ctx.beginPath()
+ ctx.moveTo(size * 0.15, size * 0.5)
+ ctx.lineTo(size * 0.85, size * 0.5)
+ ctx.lineTo(size * 0.5, size * 0.15)
+ ctx.closePath()
+ const colorHex = `#${c.toString(16).padStart(6, '0')}`
+ ctx.lineWidth = 6
+ ctx.strokeStyle = 'black'
+ ctx.stroke()
+ ctx.fillStyle = colorHex
+ ctx.fill()
+ return new THREE.CanvasTexture(canvas)
+}Then, add this in setColor (outside the changed hunk):
// after updating the main sprite texture:
if (arrowSprite) {
const mat = arrowSprite.material as THREE.SpriteMaterial
mat.map?.dispose()
mat.map = makeArrowTexture(newColor)
mat.needsUpdate = true
}🧰 Tools
🪛 GitHub Check: build-and-deploy
[failure] 150-150:
Trailing spaces not allowed
[failure] 142-142:
Trailing spaces not allowed
[failure] 135-135:
Trailing spaces not allowed
🪛 GitHub Actions: CI
[error] 135-135: Trailing spaces not allowed. (stylistic/no-trailing-spaces)
🤖 Prompt for AI Agents
In renderer/viewer/three/waypointSprite.ts around lines 135-151, the arrow
canvas texture is baked once with the initial color and never updated when
setColor/setWaypointColor is called; move the arrow texture creation into a
reusable function (e.g., makeArrowTexture(color)) inside ensureArrow so it can
be called whenever you need a fresh texture, and in setColor after updating the
main sprite texture check for arrowSprite, dispose its current material.map,
assign mat.map = makeArrowTexture(newColor) and set mat.needsUpdate = true to
update the arrow color at runtime.
There was a problem hiding this comment.
actually very good suggestion 👍
There was a problem hiding this comment.
@zardoy Thank you! I'm glad the suggestion was helpful. The arrow color synchronization issue could definitely cause confusion for users when they change waypoint colors at runtime, so extracting the texture creation into a reusable function should make the behavior much more consistent.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
zardoy
changed the title
2 participants
Add
metadataJsonto waypoints to enableonlyLeftRightarrow positioning and enhance offscreen arrow visibility with increased size and waypoint-colored fill.Summary by CodeRabbit