Skip to content
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

Refactor Image component + add ktx support #99

Merged
merged 16 commits into from
Aug 9, 2022

Conversation

stevenpaulhoward
Copy link
Contributor

@stevenpaulhoward stevenpaulhoward commented Aug 8, 2022

Refactored Image component to:

  • conditionally run JpegPng function if source file is .jpeg || .jpg || .png
  • conditionally run KTX2 function if source file is .ktx2

@vercel
Copy link

vercel bot commented Aug 8, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
spacesvr ✅ Ready (Inspect) Visit Preview Aug 9, 2022 at 9:02AM (UTC)

@alex-shortt alex-shortt changed the base branch from master to dev August 9, 2022 05:38
Comment on lines 60 to 66
export function KTX2(props: KTX2Props) {
return (
<Suspense fallback={null}>
<UnsuspensedKTX2 {...props} />
</Suspense>
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

the suspense is already being provided by the consuming component Image, no need to use it twice

Comment on lines 60 to 66
export function JpegPng(props: JpegPngProps) {
return (
<Suspense fallback={null}>
<UnsuspensedImage {...props} />
<UnsuspensedJpegPng {...props} />
</Suspense>
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

the suspense is already being provided by the consuming component Image, no need to use it twice


const modUrl = src.toLowerCase();
const IS_KTX2 = modUrl.endsWith(".ktx2");
const IS_JPEG_PNG =
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK textureloader supports more than just these formats .. webp, bmp, tga ..? think it would be cleaner to have KTX component and then an "everything else" component

@stevenpaulhoward
Copy link
Contributor Author

stevenpaulhoward commented Aug 9, 2022

Removed JpegPng naming convention and retitled the component to Texture per your suggestion

Comment on lines 60 to 62
export function Texture(props: TextureProps) {
return <UnsuspensedTexture {...props} />;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

redundant, just return the first component

Comment on lines 60 to 62
export function KTX2(props: KTX2Props) {
return <UnsuspensedKTX2 {...props} />;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

redundant, just return the first component

Comment on lines 7 to 15
type KTX2Props = {
src: string;
size?: number;
framed?: boolean;
frameMaterial?: Material;
frameWidth?: number;
innerFrameMaterial?: Material;
transparent?: boolean;
} & GroupProps;
Copy link
Collaborator

Choose a reason for hiding this comment

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

would recommend exporting the ImageProps from the parent and importing them here to keep them synchronized

import { useTexture } from "@react-three/drei";
import { GroupProps } from "@react-three/fiber";

type ImageProps = {
type TextureProps = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

would recommend exporting the ImageProps from the parent and importing them here to keep them synchronized

@alex-shortt alex-shortt merged commit 38e9249 into musehq:dev Aug 9, 2022
alex-shortt added a commit that referenced this pull request Aug 15, 2022
* Refactor Image component + add ktx support (#99)

* Refactor Image component + add ktx support

* Make Texture Component more general

* export index type & clean up syntax

Co-authored-by: Alex Shortt <alexander.shortt@gmail.com>
Co-authored-by: Muse <40903382+spacesvr@users.noreply.github.com>

* Collidable Modifier + Refactors (#100)

* add collidable modifier

* replace bvh logic with geo simplifier

* add optional raycaster prop to interactable modifier

* slight refactors to image idea

* fix up exports

* clean up keyboard layout code, don't run check if in iframe

* v2.1.0

Co-authored-by: stevenpaulhoward <40086690+stevenpaulhoward@users.noreply.github.com>
Co-authored-by: Muse <40903382+spacesvr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants