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

Dx12 dump resources only after draw calls #1952

Merged
merged 2 commits into from
Jan 28, 2025

Conversation

davidlunarg
Copy link
Contributor

No description provided.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 344132.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5780 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5780 failed.

@bradgrantham-lunarg
Copy link
Contributor

bradgrantham-lunarg commented Jan 23, 2025

I'd like @panos-lunarg to have a chance to offer comment, especially on anything that might change what other users see from Vulkan dump resources.

Comment on lines 2634 to 2635
std::string file_name_sub =
file_name + "_sub_" + std::to_string(sub_index) + (suffix == "" ? ".txt" : ("_" + suffix + ".txt"));
file_name_sub += (suffix == "" ? ".txt" : ("_" + suffix + ".txt"));
Copy link
Contributor

@locke-lunarg locke-lunarg Jan 23, 2025

Choose a reason for hiding this comment

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

Add (suffix == "" ? ".txt" : ("_" + suffix + ".txt")) twice. We could have a function for (suffix == "" ? ".txt" : ("_" + suffix + ".txt")) to reduce the duplicated code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good find. Fixed in subsequent commit. I don't have good test for this case, can you supply one?

Will squash this commit before merging to dev.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 355121.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5921 running.

@davidlunarg
Copy link
Contributor Author

davidlunarg commented Jan 24, 2025

I'd like @panos-lunarg to have a chance to offer comment, especially on anything that might change what other users might see from Vulkan dump resources.

This PR should not change the behavior of Vulkan dump resources. The --dump-resources-before-draw option is being added to dx12 and any difference in Vulkan dump resources would be a bug.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5921 failed.

@@ -71,6 +71,7 @@ struct ReplayOptions
int32_t num_pipeline_creation_jobs{ 0 };
std::string asset_file_path;
std::string dump_resources_output_dir;
bool dump_resources_before{ false };
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable is now common so it can be removed from vulkan_replay_options.h

Copy link
Contributor

@panos-lunarg panos-lunarg left a comment

Choose a reason for hiding this comment

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

I'd like @panos-lunarg to have a chance to offer comment, especially on anything that might change what other users see from Vulkan dump resources.

These changes look fine from the Vulkan perspective

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 357404.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5944 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5944 failed.

Copy link
Contributor

@bradgrantham-lunarg bradgrantham-lunarg left a comment

Choose a reason for hiding this comment

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

Approved pending test case from Locke and resolution of other reviewers feedback

Add --dump-resources-before-draw command line option: Enables
dumping resources before draw calls
@davidlunarg davidlunarg force-pushed the davidp_dx12_dump_only_after branch from 439b356 to 11c06eb Compare January 27, 2025 20:47
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 357660.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5946 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5946 failed.

@davidlunarg davidlunarg merged commit 38bd705 into LunarG:dev Jan 28, 2025
9 checks passed
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.

5 participants