Skip to content

Conversation

@willeastcott
Copy link
Contributor

  • Adds label to hotspot
  • Orange hover color
  • ...plus some other improvements

@willeastcott willeastcott requested a review from Copilot November 14, 2025 13:42
@willeastcott willeastcott self-assigned this Nov 14, 2025
@willeastcott willeastcott added the enhancement New feature or request label Nov 14, 2025
@willeastcott willeastcott merged commit e4b98bf into main Nov 14, 2025
6 checks passed
@willeastcott willeastcott deleted the new-annotations branch November 14, 2025 13:44
Copilot finished reviewing on behalf of willeastcott November 14, 2025 13:44
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the annotation system to match the style and functionality from SuperSplat Viewer. The changes include adding numeric labels to hotspots, implementing an orange hover color for better visual feedback, and refactoring the code to use shared static resources for better performance and maintainability.

  • Adds label attribute to annotations for displaying numeric identifiers on hotspots
  • Implements hover state with orange color highlighting
  • Refactors to use shared static resources (tooltip DOM, layers, mesh) instead of per-instance resources

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.

File Description
examples/assets/scripts/annotation.mjs Complete refactor of annotation system: adds label support, hover states, shared static resources, improved rendering with depth clamping, and better material handling with custom blend states
examples/annotations.html Adds label attribute (values "1" through "7") to all seven annotation instances in the jet fighter example

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 514 to 519
setTimeout(() => {
if (tooltip.style.opacity === '0') {
tooltip.style.visibility = 'hidden';
if (Annotation.tooltipDom.style.opacity === '0') {
Annotation.tooltipDom.style.visibility = 'hidden';
}
this.fire('hide');
}, 200); // Match the transition duration
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Multiple rapid calls to hideTooltip() create multiple pending setTimeout callbacks. If hideTooltip() is called multiple times quickly, earlier timeouts might incorrectly hide the tooltip even if it was shown again in between.

Consider storing the timeout ID and clearing it before setting a new one:

if (this._hideTimeoutId) {
    clearTimeout(this._hideTimeoutId);
}
this._hideTimeoutId = setTimeout(() => { ... }, 200);

Copilot uses AI. Check for mistakes.
Comment on lines 436 to 438
document.addEventListener('click', () => {
if (Annotation._activeTooltip) {
this._hideTooltip(Annotation._activeTooltip);
Annotation._activeTooltip = null;
}
this.hideTooltip();
});
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

This document-level click listener is added every time a new annotation is initialized, creating multiple duplicate listeners. This will cause hideTooltip() to be called multiple times per document click, once for each annotation instance.

Consider moving this listener to the static initialization in _initializeStatic() method to ensure it's only added once.

Copilot uses AI. Check for mistakes.
Comment on lines +456 to +478
this.app.on('prerender', () => {
if (!Annotation.camera) return;

// Clean up on entity destruction
this.on('destroy', () => {
this._tooltip.remove();
this._hotspot.remove();
if (Annotation._activeTooltip === this._tooltip) {
Annotation._activeTooltip = null;
const position = this.entity.getPosition();
const screenPos = Annotation.camera.camera.worldToScreen(position);

const { viewMatrix } = Annotation.camera.camera;
viewMatrix.transformPoint(position, vec);
if (vec.z >= 0) {
this._hideElements();
return;
}

this._updatePositions(screenPos);
this._updateRotationAndScale();

// update material opacity and also directly on the uniform so we
// can avoid a full material update
this.materials[0].opacity = Annotation.opacity;
this.materials[1].opacity = 0.25 * Annotation.opacity;
this.materials[0].setParameter('material_opacity', Annotation.opacity);
this.materials[1].setParameter('material_opacity', 0.25 * Annotation.opacity);
});
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

The 'prerender' event listener is added to this.app for every annotation instance but is never removed. This creates a memory leak where event handlers persist even after the annotation entity is destroyed. Although the 'destroy' handler on line 443 cleans up DOM elements and materials, it doesn't remove this event listener.

Consider adding this.app.off('prerender', handler) in the destroy handler, or use the built-in update() method instead of 'prerender' event for per-instance logic.

Copilot uses AI. Check for mistakes.
Comment on lines +509 to 520
hideTooltip() {
Annotation.activeAnnotation = null;
Annotation.tooltipDom.style.opacity = '0';

// Wait for fade out before hiding
setTimeout(() => {
if (tooltip.style.opacity === '0') {
tooltip.style.visibility = 'hidden';
if (Annotation.tooltipDom.style.opacity === '0') {
Annotation.tooltipDom.style.visibility = 'hidden';
}
this.fire('hide');
}, 200); // Match the transition duration
}
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

The hideTooltip() method fires a 'hide' event on this (the annotation instance), but if this method is called after the annotation has been destroyed, it will attempt to fire an event on a destroyed script instance, which could cause errors.

This can occur when hideTooltip() is called from the timeout on line 514, after the entity has been destroyed. Consider checking if the instance is still valid before firing the event, or clearing the timeout in the destroy handler.

Copilot uses AI. Check for mistakes.
// Create tooltip element
this._tooltip = document.createElement('div');
this._tooltip.className = 'pc-annotation';
// Create material the base and overlay material
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Typo in comment: "Create material the base and overlay material" should be "Create the base and overlay materials".

Suggested change
// Create material the base and overlay material
// Create the base and overlay materials

Copilot uses AI. Check for mistakes.
const screenPos = Annotation.camera.camera.worldToScreen(position);

const { viewMatrix } = Annotation.camera.camera;
viewMatrix.transformPoint(position, vec);
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

The viewMatrix.transformPoint() call modifies the position vector in place and stores the result in vec. However, position is obtained from this.entity.getPosition() which returns the entity's position vector directly (not a copy). This means the entity's actual position is being modified, which could cause unexpected behavior.

Consider creating a copy of the position before transforming it: const position = this.entity.getPosition().clone(); or use vec.copy(position) before transforming.

Copilot uses AI. Check for mistakes.
* @param {number} size - The size of the hotspot in screen pixels.
* @private
*/
static _injectStyles(size) {
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

[nitpick] The guard check if (this._styleSheet) return; was removed from _injectStyles(). Now styles are injected every time, but the method is only called once from _initializeStatic() which has its own guard. This is fine, but the method no longer protects itself from being called multiple times independently.

While this works with the current code structure, consider adding back the guard for defensive programming, or document that this method should only be called once.

Suggested change
static _injectStyles(size) {
static _injectStyles(size) {
if (Annotation.styleSheet) return;

Copilot uses AI. Check for mistakes.
Comment on lines +433 to +434
this.hotspotDom.addEventListener('pointerenter', enter);
this.hotspotDom.addEventListener('pointerleave', leave);
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Event listeners added to this.hotspotDom (click, pointerenter, pointerleave) are not explicitly removed in the destroy handler. While calling this.hotspotDom.remove() on line 444 should eventually clean up the listeners when the element is garbage collected, it's better practice to explicitly remove them to ensure immediate cleanup.

Consider storing references to the handler functions and calling removeEventListener() in the destroy handler.

Copilot uses AI. Check for mistakes.
];

if (Annotation.camera === null) {
Annotation.camera = app.root.findComponent('camera').entity;
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Finding the camera component without error handling could cause a runtime error if no camera component exists in the scene. The call app.root.findComponent('camera') could return null, and accessing .entity on null would throw an error.

Consider adding a null check or providing a more informative error message:

const cameraComponent = app.root.findComponent('camera');
if (!cameraComponent) {
    throw new Error('Annotation: No camera component found in scene');
}
Annotation.camera = cameraComponent.entity;
Suggested change
Annotation.camera = app.root.findComponent('camera').entity;
const cameraComponent = app.root.findComponent('camera');
if (!cameraComponent) {
throw new Error('Annotation: No camera component found in scene');
}
Annotation.camera = cameraComponent.entity;

Copilot uses AI. Check for mistakes.
Comment on lines +474 to +477
this.materials[0].opacity = Annotation.opacity;
this.materials[1].opacity = 0.25 * Annotation.opacity;
this.materials[0].setParameter('material_opacity', Annotation.opacity);
this.materials[1].setParameter('material_opacity', 0.25 * Annotation.opacity);
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Setting material opacity on every prerender frame (line 474-477) triggers material updates even when the opacity hasn't changed. This is inefficient if Annotation.opacity is static.

Consider only updating the material parameters when Annotation.opacity actually changes, or caching the last set opacity value to avoid redundant updates.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants