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

Fix support for 2D array images #1803

Merged
merged 5 commits into from
Jan 29, 2022
Merged

Conversation

TJYSunset
Copy link
Contributor

@TJYSunset TJYSunset commented Jan 20, 2022

  1. Describe in common words what is the purpose of this change, related
    Github Issues, and highlight important implementation aspects.

Vulkano currently doesn't support the following:

  • Generation of mipmaps for 2D array textures. Only the first layer has proper mipmapping, the other layers have blank images as their non-first mipmap levels. (If you run the example without the fix, the star and asterisk would disappear when the window is resized small enough.)
  • Single-layer array. Sounds stupid, but perfectly valid (?). In my particular use case, I used a sampler2DArray[] to store textures of different sizes/formats, with auto-generated descriptors and name-to-index lookup - which didn't quite work out at first because one of the arrays only have a single image. Yes, there will probably never exist a finished product containing single-layer array textures, but auto-inferred type that cannot be overridden is probably still a bad design.

This pull request aims to fix the issues.

Notes:

  • I have zero idea what am I doing... It seems to work and the existing tests seem to have passed, but that could be an illusion.

  • I have not tested support for 1D/3D images yet. Maybe I will commit a example containing them in this PR later.

  • Perhaps breaking the API is not a good idea?

P.S.: I am not a native English speaker, the ambiguous terminology is making my head spin and I am submitting this PR in the middle of the night... this PR is about sampler2DArray in GLSL or whatever the thing presented in this example is. Any ambiguous terms above are referring to sampler2DArray.

  1. Please put changelog entries in the description of this Pull Request
    if knowledge of this change could be valuable to users. No need to put the
    entries to the changelog directly, they will be transferred to the changelog
    files(CHANGELOG_VULKANO.md and CHANGELOG_VK_SYS.md)
    by maintainers right after the Pull Request merge.

    • Entries for Vulkano changelog:

      • **Breaking** Breaking entry description.
      • Non-breaking entry description....
    • Entries for VkSys changelog:

      • Entry 1.
      • Entry 2....
  • Fixed mipmap generation for 2D array images.
  1. Run cargo fmt on the changes.

Maybe after the reviews... (rust-lang/rustfmt#1324)

  1. Make sure that the changes are covered by unit-tests.

... um, immutable.rs and view.rs does not seems to contain any tests in the first place?

  1. Update documentation to reflect any user-facing changes - in this repository.

@Rua
Copy link
Contributor

Rua commented Jan 20, 2022

This doesn't seem like the right way to fix it at all. ImageViewBuilder already has a ty method, so I don't see the value of adding a ty value to the constructor as well. Just override the default by calling ty on the builder if you need to, that's what the builder is for. The new example and changes to ImmutableImage seem ok at first glance, but I don't agree with the changes to ImageViewBuilder.

@TJYSunset
Copy link
Contributor Author

Oops... Thanks for the heads up!

@Rua
Copy link
Contributor

Rua commented Jan 29, 2022

There's merge conflicts, could you resolve them please?

@Rua
Copy link
Contributor

Rua commented Jan 29, 2022

Thank you for the new example and the improved mipmap generation!

@Rua Rua merged commit ec4f763 into vulkano-rs:master Jan 29, 2022
Rua added a commit that referenced this pull request Jan 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants