One of preparations for compute shaders changes.
Details
Details
- Reviewers
- None
- Commits
- rP27879: Merges UID from different Vulkan device objects and unifies single type…
- Apply the patch and compile the game
- Install Vulkan SDK (if not installed)
- Add the following lines to user.cfg:
renderer.backend.debugcontext = "true" renderer.backend.debuglabels = "true" renderer.backend.debugmessages = "true" renderer.backend.debugscopedlabels = "true"
- Run the game and resize a window
- Make sure that there is no new warnings
- Add the following line to user.cfg:
renderer.backend.vulkan.disabledescriptorindexing = "true"
- Run the game and resize a window
- Make sure that there is no new warnings
Do not forget to remove added lines as they might affect performance.
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Comment Actions
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/macos-differential/7337/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/8426/display/redirect
Comment Actions
This diff weakens (syntactical) typesavety. (semanticly there is only uint32_t)
UID is to generic IMO. DeviceObjectUID might work.
I don't see where the upper bits are used as type, all UIDs are generated by GenerateNextUID which returns 1,2,3...
Why don't you use bitfields?
struct UID { u32 type : 2; u32 id : 30; };
| source/renderer/backend/vulkan/Texture.cpp | ||
|---|---|---|
| 341–345 ↗ | (On Diff #22328) | Removing this generates much duplication. |
| 343–344 ↗ | (On Diff #22328) | Why declare it at all. |
Comment Actions
Yeah, because the UID should enumerate all resources created by a device.
UID is to generic IMO. DeviceObjectUID might work.
I don't mind DeviceObjectUID.
I don't see where the upper bits are used as type, all UIDs are generated by GenerateNextUID which returns 1,2,3...
Why don't you use bitfields?
I was thinking about adding types/categories (I've added todo) to UID and I'd like to add them but I haven't came up with a satisfying solution yet (though I haven't spent much time on it).
| source/renderer/backend/vulkan/Texture.cpp | ||
|---|---|---|
| 341–345 ↗ | (On Diff #22328) | Yeah, I was thinking about that. I have no objections to pass CDevice as a parameter (though it'd be inconsistent with other device objects). |
Comment Actions
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/macos-differential/7356/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/8445/display/redirect
Comment Actions
It's better if DeviceObjectUID is a class/struct so it's not implicitly convertible from/to an integer.
| source/renderer/backend/vulkan/DeviceObjectUID.h | ||
|---|---|---|
| 36–39 ↗ | (On Diff #22352) | "It's used to describe the type" no it's not, it might be in future. |
| source/renderer/backend/vulkan/Texture.cpp | ||
| 338 ↗ | (On Diff #22352) | Can't be nullptr so it should be a reference. |
Comment Actions
Only via assignments (which we should avoid anyway). Constructors work for both:
using UID = uint32_t; // or struct UID { uint32_t value; }; UID uid{42};
| source/renderer/backend/vulkan/Texture.cpp | ||
|---|---|---|
| 338 ↗ | (On Diff #22352) | Technically it can. But yeah, I want references in the future for backend objects. |