Skip to content

Conversation

BR14Nx
Copy link
Contributor

@BR14Nx BR14Nx commented Dec 9, 2021

Referring to discussion.
Added tests, but I couldn't sucessfully run npm run test yet.
Added some description in the docs.json for explanation.
Own toString() Method for Vector2.

Works pretty much like the positionAndNormalFromPoint() method, but only returns a Vector2D.

Brian added 2 commits December 9, 2021 20:11
added testing (npm test doesn't test anything due to unknown reason)

added entry in docs.json
Copy link
Contributor

@elalish elalish left a comment

Choose a reason for hiding this comment

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

Thanks!

uvFromPoint(pixelX: number, pixelY: number): Vector2D|null {
const scene = this[$scene];
const ndcCoords = scene.getNDC(pixelX, pixelY);
scene.raycaster.setFromCamera(ndcCoords, scene.getCamera());
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind shifting this over next to positionAndNormalFromPoint in annotations.ts instead? I think following that logic will also clean this up a shade (no need to access the raycaster out here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I was doing this in the beginning, but then I was not sure if it really fits under "annotations" since I couldn't see a use case relating the hotspots. Though I changed that now.

'uvFromPoint returns null when intersect fails', async () => {
await loadModel(ASTRONAUT_GLB_PATH);

await timePasses(1000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I was following the test from "materialFromPoint()" for coherence. I removed it, and just check that the UV-coords are within 0-1 inside the positionAndNormalFromPoint() test.


export interface Vector2D {
x: number
y: number
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be u and v instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed that now, but wouldn't it be maybe better for other use cases in the future if it stays x and y? So it can be reused without creating confusion?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, but I think we'll cross that bridge when we come to it. I don't see a lot of need to return {x, y}, so hopefully it's a non-issue.

Removed method from scene-graph and return uv from previous existing positionAndNormalfromPoint().
@BR14Nx
Copy link
Contributor Author

BR14Nx commented Dec 11, 2021

I made the changes you requested. Let me know if it suits your proposals.

Copy link
Contributor

@elalish elalish left a comment

Choose a reason for hiding this comment

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

Thanks, I like that better. The method name ought to be more generic, but it's not worth a breaking change. Just a little cleanup left.

}

if (hit.uv == null) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't want this; a model can lack UV coordinates, in which case uv = null makes sense, and it shouldn't destroy the rest of the result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made some according changes.

return {position: hit.point, normal: hit.face.normal, uv: hit.uv};
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Forgot to delete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed

also deleted old comments
Copy link
Contributor

@elalish elalish left a comment

Choose a reason for hiding this comment

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

Thanks!

@elalish elalish merged commit c793b92 into google:master Dec 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants