Skip to content

Conversation

bandures
Copy link
Contributor

Purpose of this PR

Fix for https://fogbugz.unity3d.com/f/cases/1372769/
It adds attributes to profiler wrappers to hide them in deep profiler so that they don't break begin/end block for profiler markers. It uses a newly introduced attribute added in this PR https://ono.unity3d.com/unity/unity/pull-request/137412/_/profiler/bugfix/case1372769-calls-are-not-tracked-in-deep-profiling-mode

Testing status

Manually tested using HDRP project

@github-actions
Copy link

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://unity-ci.cds.internal.unity3d.com/project/902/
Search for your PR branch using the search bar at the top, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

SRP Core
You could run ABV on your branch before merging your PR, but it will start A LOT of jobs. Please be responsible about it and run it only when you feel the PR is ready:
/jobDefinition/.yamato%252F_abv.yml%2523all_project_ci_trunk
Be aware that any modifications to the Core package impacts everyone in the Graphics repo so please discuss the PR with your lead.

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

@github-actions github-actions bot added the SRP label Jan 10, 2022
@github-actions
Copy link

It appears that you made a non-draft PR!
Please convert your PR to draft (button on the right side of the page).
See the PR template for more information.
Thank you!

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to the Unity SRP repo!
Please make sure to fill out the PR template as best you can to give reviewers as much information as possible.
If you have any questions (and you are a Unity employee) go to "#devs-renderpipe"

Copy link
Contributor

@arttu-peltonen arttu-peltonen left a comment

Choose a reason for hiding this comment

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

Adding @JulienIgnace-Unity to review and @Unity-Technologies/gfx-qa-hdrp to verify.

I'm not sure I understand why hiding these samplers in deep profiling mode fixes the problem, maybe Anton or Julien can explain? If it does fix it, and we see all the SRP brackets also in deep profiling mode, seems like a good change :)

@bandures please wait for the trunk fix to land, and then re-run the Safety Net yamato playlist, currently it fails to compile because the attribute doesn't exist yet. This job must be green before merging.

@arttu-peltonen arttu-peltonen requested review from a team and JulienIgnace-Unity January 11, 2022 08:47
@arttu-peltonen
Copy link
Contributor

@JulienIgnace-Unity also can you confirm that backporting this is not necessary? To me at least it would seem like unnecessary hassle to do so since it depends on a trunk change.

@JulienIgnace-Unity
Copy link
Contributor

@arttu-peltonen yeah not sure we need to backport this unless requested. We'd need to have the C++ PR backported first anyway.

Copy link
Contributor

@RemyUnity RemyUnity left a comment

Choose a reason for hiding this comment

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

The change is pretty confined, trusting dev testing 🟢

@sebastienlagarde sebastienlagarde merged commit 8ad7300 into master Feb 21, 2022
@sebastienlagarde sebastienlagarde deleted the profiler/bugfix/case1372769-calls-are-not-tracked-in-deep-profiling-mode branch February 21, 2022 16:30
vincent-breysse added a commit that referenced this pull request Feb 25, 2022
…tributes to profiler wrappers to hide them in deep profiler (#6702)"

This reverts commit 8ad7300.
vincent-breysse added a commit that referenced this pull request Mar 2, 2022
…tributes to profiler wrappers to hide them in deep profiler (#6702)"

This reverts commit 8ad7300.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants