Adding PlaneGeometry #887
Adding PlaneGeometry #887
Conversation
|
| @@ -0,0 +1,163 @@ | |||
| import React, { useRef, useState } from 'react' | |||
| import ReactDOM from 'react-dom/client' | |||
| import { getSession } from '@webspatial/react-sdk' | |||
There was a problem hiding this comment.
'getSession' should not be available from react-sdk. This is a legacy bug. We'll fix it soon.
It seems that you have created testcase for core-sdk part, and missing react-sdk test code
| @@ -0,0 +1,2 @@ | |||
| declare const __WEBSPATIAL_REACT_SDK_VERSION__: string | |||
There was a problem hiding this comment.
It looks alreadly exist in packages/react/src/types/global.d.ts
There was a problem hiding this comment.
why do we need another globals.d.ts instead of using global.d.ts
There was a problem hiding this comment.
I think I missed this, some reason my IDE was misconfigured.
| // Minimal ambient type so AndroidPlatform.ts compiles in non-Android envs | ||
| declare global { | ||
| interface Window { | ||
| SpatialId?: unknown // keep optional & unknown to avoid leaking shape | ||
| } | ||
| } | ||
| export {} |
There was a problem hiding this comment.
Why is this required? Maybe it should be a separate commit it's part of the Android work.
There was a problem hiding this comment.
Hmm, when I try to push, it gives me an error if I don't have this:
adapter/android/AndroidPlatform.ts(57,25): error TS2339: Property 'SpatialId' does not exist on type 'Window'.
(this is unrelated to this work though, so I would agree)
| }, | ||
| ref, | ||
| ) => { | ||
| const ctx = useRealityContext() |
There was a problem hiding this comment.
Best to make ctx not optional
| const ctx = useRealityContext() | |
| const ctx = useRealityContext()! |
| return ent | ||
| } catch (error) { | ||
| await manager.dispose() | ||
| return null as any |
There was a problem hiding this comment.
Why does this have to be cast as any?
There was a problem hiding this comment.
I am following similar logic to BoxEntity, I believe it's to make tsc happy
| nil | ||
| } else { | ||
| return nil | ||
| if let width = props.width, let height = props.height, let depth = props.depth { |
There was a problem hiding this comment.
I think this changes the expected behaviour. Before we expected the width, height, and depth for a BoxGeometry and would otherwise crash but now you're silently ignoring them and returning nil.
There was a problem hiding this comment.
My reasoning was that we should not fail in this layer, and instead have the JS/react layer fail, and return an error. However, if this was intended behavior I can revert this.
Summary
PlaneGeometryto core and React for creating and displaying planar surfaces.Changes
SpatialPlaneGeometryandCreateSpatialGeometryCommand(plane variant), addcreatePlaneGeometry(...)onSpatialSession, export types and factory.PlaneEntityfor consuming plane geometries.apps/test-server/src/reality/geometryfor a standard plane and a rounded plane, withspatialtapevent logging. as well as boxUsage
session.createPlaneGeometry({ width, height, ... })and attach to a model/entity.PlaneEntityand handlespatialtapvia props or coreaddEvent.Test