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

Initial work for WebGPU and refactor GPU namespace #11

Open
wants to merge 1 commit into
base: cpp
Choose a base branch
from

Conversation

andraantariksa
Copy link
Member

No description provided.


"include/tempeh/gpu/vk/device.hpp"
"include/tempeh/gpu/vk/vk.hpp"
"include/tempeh/gpu/vk/volk.h")
Copy link
Member

Choose a reason for hiding this comment

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

volk.h should not be here

Copy link
Member

@native-m native-m left a comment

Choose a reason for hiding this comment

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

I revert this changes and will never merge this to the master. If you are using the master branch as your base for the implementation, it's incomplete. Please use vulkan-impl branch as the base of WebGPU backend implementation, it contains complete (almost) interfaces.

#include "vk.hpp"

namespace Tempeh::GPU
namespace Tempeh::GPU::Vk
Copy link
Member

@native-m native-m Feb 9, 2022

Choose a reason for hiding this comment

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

Do not add too many namespaces.
Seriously, you can always put the name of the backend at the end of the derived class name.
Deep namespace is unnecessary in this case.
These classes are not going to be visible to the end-user anyway, so adding the namespace is completely useless.

Copy link
Member Author

Choose a reason for hiding this comment

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

Namespace good. If its too long, you can just use alias

Copy link
Member

@native-m native-m Feb 9, 2022

Choose a reason for hiding this comment

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

Not good, alias can add redundant lines. It's just not worth it.
Why add deep namespaces when can just DeviceVK. Tbh, it's not worth it in this case.

@native-m native-m closed this Feb 9, 2022
using Box = std::unique_ptr<T>;

template<typename T>
FUNCTION_ALIAS(make_rc, std::make_shared<T>);
Copy link
Member

@native-m native-m Feb 9, 2022

Choose a reason for hiding this comment

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

make_shared & make_unique requires you to provide the constructor parameters, this is probably wont work for classes that don't have a default constructor.

@native-m native-m reopened this Feb 9, 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.

None yet

2 participants