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

Small batch mode fixes #1036

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

Conversation

tomadamatkinson
Copy link
Collaborator

@tomadamatkinson tomadamatkinson commented Apr 26, 2024

Description

A series of small batch mode fixes that i found when running batch mode. This does not fix validation errors but does fix segfaults / assertions.

Added force_render which allows batch mode to override the update if focused functionality so that you can multi task whilst running batch mode in the background.

Tested on Linux. Samples may still have existing issues on other platforms

General Checklist:

Please ensure the following points are checked:

  • My code follows the coding style
  • I have reviewed file licenses
  • I have commented any added functions (in line with Doxygen)
  • I have commented any code that could be hard to understand
  • My changes do not add any new compiler warnings
  • My changes do not add any new validation layer errors or warnings
  • I have used existing framework/helper functions where possible
  • My changes do not add any regressions
  • I have tested every sample to ensure everything runs correctly
  • This PR describes the scope and expected impact of the changes I am making

Note: The Samples CI runs a number of checks including:

  • I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
  • My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions

If this PR contains framework changes:

  • I did a full batch run using the batch command line argument to make sure all samples still work properly

asuessenbach
asuessenbach previously approved these changes Apr 29, 2024
@SaschaWillems
Copy link
Collaborator

Aside from gary's comment this looks fine. This will be very useful for maintainers, esp. being able to have the batch run continue when inactive. That way I can finally run it on my second screen while doing other stuff 👍🏻

@SaschaWillems SaschaWillems self-requested a review May 1, 2024 15:45
SaschaWillems
SaschaWillems previously approved these changes May 1, 2024
@SaschaWillems
Copy link
Collaborator

SaschaWillems commented May 28, 2024

@tomadamatkinson: Are you able to fix what Gary noted? Getting those fixes merged would make future PRs a lot easier. If you can't find the time for that, I'm happy to jump in :)

@tomadamatkinson
Copy link
Collaborator Author

@SaschaWillems @gary-sweet this should be resolved now!

@SaschaWillems
Copy link
Collaborator

Awesome. Thank you very much :)

@SaschaWillems SaschaWillems self-requested a review May 30, 2024 10:19
SaschaWillems
SaschaWillems previously approved these changes May 30, 2024
Copy link
Collaborator

@SaschaWillems SaschaWillems left a comment

Choose a reason for hiding this comment

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

LGTM

@gary-sweet
Copy link
Contributor

I'm still seeing some odd display in command_buffer_usage sadly:

20240530_114004

@tomadamatkinson
Copy link
Collaborator Author

@gary-sweet thats a weird one...

Just to check, you've pulled the up to date version of the branch. I did rebase on main.

Do you see this behaviour with any other sliders? The slider I changed is now inline with the other slider usage

@gary-sweet
Copy link
Contributor

Just to check, you've pulled the up to date version of the branch. I did rebase on main.

Yes

Do you see this behaviour with any other sliders? The slider I changed is now inline with the other slider usage

Well, if I run command_buffer_usage on main, I just get:
vulkan_samples: third_party/imgui/imgui.cpp:9907: bool ImGui::ItemAdd(const ImRect&, ImGuiID, const ImRect*, ImGuiItemFlags): Assertion 'id != window->ID && "Cannot have an empty ID at the root of a window. If you need an empty label, use ## and read the FAQ about how the ID Stack works!"' failed.

All the other sliders appear to have the label to the right of them (which I think is how they're supposed to be looking at the screenshots from those samples).

@tomadamatkinson
Copy link
Collaborator Author

Interesting, I'll take another look!

@gary-sweet
Copy link
Contributor

gary-sweet commented May 30, 2024

Interesting, I'll take another look!

Changing the slider code in command_buffer_usage.cpp to:

ImGui::SliderInt("##secCmdBufs", &gui_secondary_cmd_buf_count, 0, max_secondary_command_buffer_count, "Secondary CmdBuffs: %d");

solves the problem for me. The ##secCmdBufs ensures there is a unique ID associated with the slider, and the %d then starts working.

@tomadamatkinson
Copy link
Collaborator Author

Ah it was an issue with my setup. Looks like the binary path to Vulkan Samples has now changed on MacOS. My run command was pointing at an older version 🤦

Screenshot 2024-05-30 at 20 20 18

fix %d

fix label

fix
Copy link
Contributor

@gary-sweet gary-sweet left a comment

Choose a reason for hiding this comment

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

Looks good now, thanks.

@SaschaWillems SaschaWillems added the bug Something isn't working label Jun 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants