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.

Sign up for GitHub

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

Jump to bottom

Decode and encode Texture2D from webp, png, jpg formats #292

Merged
merged 19 commits into from
Jun 3, 2023
Merged

Decode and encode Texture2D from webp, png, jpg formats #292

merged 19 commits into from
Jun 3, 2023

Conversation

kkard2
Copy link
Contributor

@kkard2 kkard2 commented Jun 3, 2023

closes #281

@kkard2 kkard2 requested a review from Vixenka June 3, 2023 11:50
Vixenka
Vixenka requested changes Jun 3, 2023
View reviewed changes
Copy link
Member

@Vixenka Vixenka left a comment

Choose a reason for hiding this comment

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

When creating the task, I also said to add the From/To methods to Texture2D, which would use CpuTexture2D for now. This missing.

NoiseEngine.Native/src/interop/rendering/cpu_texture_interop.rs Outdated Show resolved Hide resolved
NoiseEngine/Rendering/TextureFormat.cs Outdated Show resolved Hide resolved
NoiseEngine/Rendering/Utils/TextureFormatUtils.cs Outdated Show resolved Hide resolved
NoiseEngine/Rendering/CpuTexture2D.cs Outdated Show resolved Hide resolved
NoiseEngine/Rendering/CpuTexture2D.cs Outdated Show resolved Hide resolved
NoiseEngine/Rendering/TextureFormat.cs Show resolved Hide resolved
NoiseEngine.Native/src/rendering/encoding.rs Outdated Show resolved Hide resolved
NoiseEngine.Native/src/rendering/cpu.rs Outdated
pub enum TextureFileFormat {
Png,
Jpeg,
Webp
Copy link
Member

Choose a reason for hiding this comment

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

Should not it be WebP?

NoiseEngine.Native/src/rendering/cpu.rs Outdated Show resolved Hide resolved
NoiseEngine/Rendering/CpuTexture2D.cs Outdated Show resolved Hide resolved
@kkard2
Copy link
Contributor Author

kkard2 commented Jun 3, 2023

imo it's not a good idea to maintain 2 apis that do exactly the same thing.
i don't think saving to file is very common operation, and even if it is, CpuTexture2D.FromTexture2D(texture).ToPng() is not that bad

@kkard2 kkard2 requested a review from Vixenka June 3, 2023 13:24
Vixenka
Vixenka requested changes Jun 3, 2023
View reviewed changes
Copy link
Member

@Vixenka Vixenka left a comment

Choose a reason for hiding this comment

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

Maintaining similar API in Texture2D allows us to read the developer's behavior. So we can better implement decoding/encoding such textures in the future using GPU accelarated encoders or technologies like DirectX DirectStorage.

NoiseEngine/Rendering/CpuTexture2D.cs Outdated Show resolved Hide resolved
@kkard2 kkard2 requested a review from Vixenka June 3, 2023 15:39
@kkard2 kkard2 requested a review from Vixenka June 3, 2023 17:51
@kkard2 kkard2 merged commit a702c5b into master Jun 3, 2023
@kkard2 kkard2 deleted the feature/281/decode-encode-texture branch June 3, 2023 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Vixenka Vixenka Vixenka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Decode and encode Texture2D from webp, png, jpg formats
2 participants
@kkard2 @Vixenka