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

Create cubemap from file #93

Merged

johannesugb merged 56 commits into master from create_cubemap_from_file

May 18, 2021

Collaborator

stf976 commented Feb 27, 2021

pull request for implementation of #62 for review;
API documentation not yet included

stf976 added 16 commits

November 25, 2020 11:39


          partial implementation of cubemap support

c0c07af


          finish first working draft of cubemap texture support,

a819e2f

including example application


          in texture_cubemap example, transform left-handed cubemap coordinates…

6cbbb05

… to right-handed world space

and vice versa using modelView matrices instead of in shader code;
add comments explaining coordinate system transformations


          first (incomplete) draft for loading cubemap from six individual files

b451822


          adapt texture_cubemap example and load_cubemap_from_file after rebasi…

6fe145f

…ng branch on current master


          add image_resource_t helper class for image loading library abstraction;

82101f3

implement image_resource_t classes for GLI and stb_image loaders;
add factory method create_image_resource_from_file();
implement composite_cubemap_image_resource_t class for creating cubemap from individual files;
update texture_cubemap example


          partial implementation of cubemap support

726afae


          finish first working draft of cubemap texture support,

e5ae27f

including example application


          in texture_cubemap example, transform left-handed cubemap coordinates…

e22b572

… to right-handed world space

and vice versa using modelView matrices instead of in shader code;
add comments explaining coordinate system transformations


          first (incomplete) draft for loading cubemap from six individual files

35c2520


          adapt texture_cubemap example and load_cubemap_from_file after rebasi…

7cdcfe7

…ng branch on current master


          add image_resource_t helper class for image loading library abstraction;

f0d3dd6

implement image_resource_t classes for GLI and stb_image loaders;
add factory method create_image_resource_from_file();
implement composite_cubemap_image_resource_t class for creating cubemap from individual files;
update texture_cubemap example


          add cubemap sample texture in ktx and dds format

4bdd96f

add links to source


          implement image_resource as bridge pattern to facilitate caching

8ab4b4b

simplify image_resource interfaces
fix screen resizing in texture_cubemap example after update
add caching to texture_cubemap example
add loading of dds texture to texture_cubemap example


          update revision of auto_vk

17491e3


          Merge branch 'create_cubemap_from_file' of https://github.com/cg-tuwi…

b3be9df

…en/Gears-Vk into create_cubemap_from_file

johannesugb requested changes

View reviewed changes

Member

johannesugb left a comment

Sorry if some of the comments might appear a bit nit-picky, but the framework shall serve users well and features shall be implemented in similar ways, so that users get the chance to "feel at home" with the framework. That means that new features must follow established principles and patterns.

I know that this can be a bit cumbersome from time to time and refactoring will take some time, but it is for a good purpose. It is more important to implement new features in a well and consistent way so that the frameowork feels like as if made from one piece than to implement a lot of new features and each of them requires different usage patterns.

If you have further questions, please do not hesitate to discuss directly at the issues or in Slack!

examples/texture_cubemap/source/texture_cubemap.cpp Outdated Show resolved Hide resolved

examples/texture_cubemap/source/texture_cubemap.cpp Outdated Show resolved Hide resolved

examples/texture_cubemap/source/texture_cubemap.cpp Outdated Show resolved Hide resolved

examples/texture_cubemap/source/texture_cubemap.cpp Outdated Show resolved Hide resolved

examples/texture_cubemap/source/texture_cubemap.cpp Outdated Show resolved Hide resolved

framework/include/image_resource.hpp Outdated Show resolved Hide resolved

examples/texture_cubemap/source/texture_cubemap.cpp Outdated Show resolved Hide resolved

examples/texture_cubemap/source/texture_cubemap.cpp Outdated Show resolved Hide resolved

examples/texture_cubemap/source/texture_cubemap.cpp Outdated Show resolved Hide resolved

examples/texture_cubemap/source/texture_cubemap.cpp Outdated Show resolved Hide resolved

stf976 and others added 13 commits

March 20, 2021 17:59


          add license information about yokohama_at_night textures

b08d507


          remove commented-out code

5e05a02


          follow naming conventions

4b8155b

Co-authored-by: Johannes Unterguggenberger <[email protected]>


          rename sRGB, HDR parameters

cd86eb4


          add virtual constructor to image_resource_t

1c6d867

make conversion constructors of image_resource_base_t explicit
make variables and parameters const and auto if possible
prevent variable use after std::move


          Merge branch 'create_cubemap_from_file' of https://github.com/cg-tuwi…

43607a5

…en/Gears-Vk into create_cubemap_from_file


          delete special functions for image_resource_base_t

278fd60

make conversion constructors explicit for image_resource_t classes


          follow naming conventions

8f0d4ca

Co-authored-by: Johannes Unterguggenberger <[email protected]>


          Apply suggestions from code review

2d601bf

follow naming conventions
use gvk::owned instead of std::move

Co-authored-by: Johannes Unterguggenberger <[email protected]>


          use avk::resource_reference for type of image_resource instead of std…

41cebe9

…::unique_ptr, adapt code accordingly

add create_image_resource() helper functions
fix compilation errors in texture_cubemap example


          use avk::owning_resource for type of image_resource instead of std::u…

8a4cf47

…nique_ptr, adapt code accordingly

add create_image_resource() helper functions
fix compilation errors in texture_cubemap example


          Merge branch 'create_cubemap_from_file' of https://github.com/cg-tuwi…

d0e8fb1

…en/Gears-Vk into create_cubemap_from_file


          add explanation of memory, resource leaks for [[nodiscard]] to guidel…

cff294d

…ines

stf976 and others added 9 commits

April 25, 2021 17:24


          code cleanup in cubemap_texture example

9a5b1b4

Co-authored-by: Johannes Unterguggenberger <[email protected]>


          remove initialization of removed mInitTime variable

edad486


          use consistent parameter names, camel case

357b886

Co-authored-by: Johannes Unterguggenberger <[email protected]>


          use consistent variable names

9efcfc1


          use create_vertex_and_index_buffers, create_normals_buffer for model …

7cead62

…loading in texture_cubemap example


          remove owning_resource wrapper from image_data

Co-authored-by: Johannes Unterguggenberger <[email protected]>


          remove owning_resource from image_data

c20685b


          use only a single descriptor cache for both pipelines

373e123


          add mirror_matrix and cancel_translation_from_matrix helper functions

54cd2c9

johannesugb requested changes

View reviewed changes

framework/include/math_utils.hpp Outdated Show resolved Hide resolved

framework/include/math_utils.hpp Outdated Show resolved Hide resolved

framework/src/math_utils.cpp Outdated Show resolved Hide resolved

framework/src/math_utils.cpp Outdated Show resolved Hide resolved

stf976 and others added 8 commits

May 10, 2021 20:48


          use enum to define axis in mirror_matrix

321d016

follow naming guidelines for parameters


          update auto_vk ref

14bc637


          Merge branch 'master' of https://github.com/cg-tuwien/Gears-Vk into c…

131a95d

…reate_cubemap_from_file


          update description of texture_cubemap shaders, add newline

d40fdfd

Co-authored-by: Johannes Unterguggenberger <[email protected]>


          don't explicitly specify image format for image view

ab98735


          Merge branch 'create_cubemap_from_file' of https://github.com/cg-tuwi…

c527559

…en/Gears-Vk into create_cubemap_from_file


          use enum struct instead of old style enum

0301fb0

fix parameter type


          - Restructured texture_cubemap example a bit by introducing `enum str…

4f03ea2

…uct options`. This should improve readability.

- Removed useless `updater` calls and restructured the remaining code to be clearer.
- Added comments to the functions in `math_utils.hpp` that are added with this PR
- Removed default parameter value for `mirror_matrix`'s `principal_axis` parameter. There is not really a justification why any of the axes should be the default. The user must be precise anyways. Also removed the parameter's `const` declaration.

Member

johannesugb commented May 12, 2021

@stf976 Please have a look at the changed that I have pushed and see if you agree!

Collaborator Author

stf976 commented May 13, 2021

Changes look good. Shouldn't function arguments always be const if possible? I have already prepared documentation for the math_util functions as part of the API documentation, will update it when it is finished.

Member

johannesugb commented May 13, 2021

Changes look good. Shouldn't function arguments always be const if possible? I have already prepared documentation for the math_util functions as part of the API documentation, will update it when it is finished.

Actually, const only makes sense for reference or pointer arguments to functions because it tells the user of the function that the passed value will not be modified.
However, arguments that are passed by value do not need to be const since a copy has been made anyways. Declaring by-value arguments const just takes away some (potential) flexibility for the implementation of the function. If the function wanted to overwrite the variable (for whatever reason) it could do so -- but only if the argument a.k.a. variable is non-const.


          Linked to latest Auto-Vk master which includes necessary changes for …

593337e

…this PR.

Member

johannesugb commented May 17, 2021 •

edited by stf976

Loading

@stf976 There is one more issue:
Some image files fail to load. This can be reproduced easily by running the model_loader example.

It fails (in DEBUG mode!) in image_data.cpp, line 217 at the following assert:

assert(can_flip());

I think, the fix is trivial, because the code has actually no reason to go there. GLI fails to load a .png texture and still tries to flip, as far as I can tell.

Before re-requesting a review, please ensure that the following examples run fine:

compute_image_processing in DEBUG and in RELEASE mode
model_loader in DEBUG and in RELEASE mode
orca_loader in DEBUG and in RELEASE mode
ray_query_in_ray_tracing_shaders in DEBUG and in RELEASE mode
ray_tracing_with_shadows_and_ao in DEBUG and in RELEASE mode

If you don't have a ray tracing capable GPU, I'll test them before merging the PR.

stf976 added 2 commits

May 17, 2021 20:45


          don't try to flip gli texture if it wasn't loaded successfully

4546deb


          Merge branch 'create_cubemap_from_file' of https://github.com/cg-tuwi…

51aa1c1

…en/Gears-Vk into create_cubemap_from_file

Collaborator Author

stf976 commented May 17, 2021

@stf976 There is one more issue:
Some image files fail to load. This can be reproduced easily by running the model_loader example.

Fixed, the model_loader works again now. I also tested the other examples where possible, but don't have a GPU that supports RTX to test the last two.

johannesugb added 2 commits

May 18, 2021 09:26


          Merge branch 'master' into create_cubemap_from_file

16cd3d2


          Fixed RELEASE-mode project config in texture_cubemap.vcxproj.user

76c7d0e

johannesugb merged commit 08d4c97 into master

johannesugb deleted the create_cubemap_from_file branch

May 18, 2021 09:01

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet