-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat(texture): support selecting textures #356
Conversation
This pull request is automatically being deployed by Amplify Hosting (learn more). |
@belom88 please review |
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.
It is unnecessary complexity with textures initialization and adding textures via uv-debug-texture-slice
. The responsibilities of slices are clear:
icon-list-slice
- manage lists;uv-debug-texture-slice
- load the texture when the url has changed.
How they should work together:
- Init the list directly in
icon-list-slice
. It is OK that there will be "list of textures" in this slice initialized. It is not a generic component for a UI library. All add and remove list item logic should dispatch directly to this slice; uv-debug-texture-slice
doesn't know about multiple textures. It stores only a loadedImageBitmap
. Addurl
argument tofetchUVDebugTexture
. Make sure that the texture is null during requests. Probably, need to track request status (idle
,loading
,succeded
,failed
);- We should have a
useEffect
inDeckglWrapper
- it should dispatchfetchUVDebugTexture
when selected texture has changed
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.
const uvDebugTexture = useAppSelector(selectUVDebugTexture(imageUrl)); | ||
const uvDebugTextureRef = useRef<ImageBitmap | null>(null); | ||
uvDebugTextureRef.current = uvDebugTexture; | ||
const [needTransitionToTileset, setNeedTransitionToTileset] = useState(false); | ||
|
||
const [forceRefresh, setForceRefresh] = useState(false); |
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.
forceRefresh
is changed but never used
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.
After the texture is replaced in the tileset (in useEffect) we change the variable forceRefresh in order to start re-rendering.
action: PayloadAction<{ imageUrl: string; image: ImageBitmap | null }> | ||
action: PayloadAction<{ | ||
imageUrl: string; | ||
image: ImageBitmap | null; |
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.
Does it make sense to push null
to image
?
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.
Removed null from the type.
No description provided.