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

Some suggestions #20

Closed
thatname opened this issue Dec 11, 2023 · 2 comments
Closed

Some suggestions #20

thatname opened this issue Dec 11, 2023 · 2 comments

Comments

@thatname
Copy link

Hi, this library is very cool !
After using it, I have some suggestions:

  • Make VKResource and MTLResource abstract, move implementation to image/buffer/sampler/accel subclasses. After change:
    • Resource subclasses cost less memory.
    • Most branching logic can be removed, because all methods of Resource are virtual.
  • Add build targets for each underlying graphics API, then we can build dynamic libraries.
  • Add CopyTextureToBuffer(), which is useful.
  • Make debug layer optional. For now, debug layer is always enabled when debugger attached, which is slow.
  • Add depth range parameter for SetViewport().
  • Remove glm from acceleration structure dependency, user may prefer other math library.
  • Required CMake version is too high(3.28), lower to 3.1x?
  • Use CMake fetchContent instead of git submodules for external dependencies, which is more flexible.
    I've forked the repo, trying to accomplish all the above. I'll open a pull request if helpful.
@andrejnau
Copy link
Owner

Hi! I'm glad you liked it. And thanks for pointing out some issues.

Yes, there are some functions missing and probably not just CopyTextureToBuffer or depth range. As well as some code need a bit refactoring.

Add build targets for each underlying graphics API, then we can build dynamic libraries.

I will add target FlyCube that contains implementation for all graphics backends. Is this what you offer?

Required CMake version is too high(3.28), lower to 3.1x?

3.28 was required to link with MoltenVK.xcframework. I ended up replacing the version with 3.22 in bd18fbd . Is this still too high?

Use CMake fetchContent instead of git submodules for external dependencies, which is more flexible.

I'll take a look at fetchContent.

@thatname
Copy link
Author

thatname commented Dec 13, 2023

I will add target FlyCube that contains implementation for all graphics backends. Is this what you offer?

I mean one target for one backend. If there is a D3D12 DLL and a Vulkan DLL, An app can choose to load only one on start, thus consume less RAM.

3.28 was required to link with MoltenVK.xcframework. I ended up replacing the version with 3.22 in bd18fbd . Is this still too high?

3.22 is OK. For Ubuntu v22.04 LTS, current version of CMake package from offical source is 3.22.1-1.

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

No branches or pull requests

2 participants