-
Notifications
You must be signed in to change notification settings - Fork 22
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.
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
Autoregister images #909
Autoregister images #909
Conversation
@@ -182,6 +183,25 @@ class OutputContext implements CompositorOutputContext { | |||
{} | |||
); | |||
} | |||
public async registerImage(imageId: number, imageSpec: any) { | |||
const imageRef = { | |||
type: 'image-local', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
output-local
in inputs means that this "input" is local/scoped/limited/assigned to that specific output, so with this naming output-local
would make sense.
I guess we could use output-local-input
and output-local-image
to make sure they ae different, or maybe output-specific
@@ -35,7 +35,7 @@ class LocallySpawnedInstance implements CompositorManager { | |||
this.port = opts.port; | |||
this.workingdir = opts.workingdir ?? path.join(os.tmpdir(), `live-compositor-${uuidv4()}`); | |||
this.executablePath = opts.executablePath; | |||
this.enableWebRenderer = opts.enableWebRenderer; | |||
this.enableWebRenderer = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
}; | ||
}, [props.source]); | ||
|
||
if (!isImageRegistered) return createElement(View, {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't use if statements without {}
it is very easy to add new line and introduce wierd bugs
|
||
export type ImageProps = Omit<ComponentBaseProps, 'children'> & | ||
( | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add runtime check to make sure only one is provided
return createElement(View, {}); | ||
} | ||
|
||
type ImageSceneBuliderProps = Omit<ImageProps, 'imageId'> & { imageId: string }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe move that up? above first use
const ctx = useContext(LiveCompositorContext); | ||
const [imageId, setImageId] = useState(0); | ||
const [isImageRegistered, setIsImageRegistered] = useState(!!props.imageId); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to handle when component props change between imageId and source e.g. reset isImageRegistered to false
|
||
function Image(props: ImageProps) { | ||
const ctx = useContext(LiveCompositorContext); | ||
const [imageId, setImageId] = useState(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe internalImageId or autoImageId because it is confusing to distinguish this id from props one
|
||
void (async () => { | ||
try { | ||
if (!assetType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move that outside this async function
#902