Unity-Technologies / ProjectAuditor

Project Auditor is an experimental static analysis tool for Unity Projects.
Other
806 stars 66 forks source link

Shaders improvements #77

Closed mtrive closed 2 years ago

mtrive commented 2 years ago

This (draft) PR adds additional information to Shaders and Shader Variants views.

Shaders:

shaders

Shader Variants:

Let me know what you think and I will iterate when I am back from PTO.

mtrive commented 2 years ago

Hi @unitystevem,

Shaders view

In my test project for Project Auditor shader reporting (the one that builds a custom shader into an AssetBundle using a simple custom build script), the Size column shows that the shader is 0 bytes, even though it shows 72 variants included in the build.

The problem is that the shader size info is not contained in the build report generated after building the project. I'll see if it's possible to get that info from the AssetBundles build report.

The same project shows all of the hidden / built-in shaders as being 7.07KB despite them being different shaders with different variant counts. Let me know if you’d like another copy of the project and a copy of the Editor log after a fresh build in a clean project (no Library folder, no Assets/StreamingAssets folder, which is where the AssetBundle gets built to).

The ~7KB is the size of all shaders in _Resources/unity_builtinextra. Unfortunately, the build report does not give us smaller granularity sizes for those shaders so I think we need to either better inform the user or hide those shaders to avoid confusion.

Shaders view

Platform Keywords (and Keywords, to some extent) are very long strings, even in simple test cases. Could we have a bottom panel (like the inverse call stack panel in the Code Issues View) that shows a text-wrapped, scrollable copy of the currently-selected cell or something?

I agree this is not a good user experience. Let's assume we can have a text-wrapped panel. The problem is that we can only select a row, as opposed to a specific single cell. Maybe we could display the content of the cell the cursor is hovering on?

Alternatively, what do you think about adding either:

shader-inspector

As a more general comment (and one that’s less serious than the two above), I’m not entirely convinced that the UNITY_2018_2_OR_NEWER or VARIANTS_ANALYSIS_SUPPORT defines are needed any more. Are we actively supporting versions that have dropped out of LTS? If so, for how long? Stripping those out is probably a different pull request to this one, though.

definitely a change that would require a different PR but worth bringing up. The short answer is that we don't support versions that dropped out of LTS, however, we try to preserve backwards compatibility if it requires low effort. This has been the case so far. Having said that, there is a bit of overhead in this process, mainly because we don't have automated testing for 2018 or earlier versions. Let's try to look at the analytics data and continue the conversation.

unitystevem commented 2 years ago

The problem is that the shader size info is not contained in the build report generated after building the project. I'll see if it's possible to get that info from the AssetBundles build report.

Let me know if you need a copy of the test project and/or the Editor logs from building. The shader size is indeed reported correctly in the AssetBundle build. Is the AB build report where Project Auditor is finding out about the shader in the first place? Or is it from an IPreprocessShaders callback before the build report is complete and the size is known?

Hopefully you can get the information from the AB build report. If not, maybe Project Auditor should report unknown shader sizes as "unknown" rather than "0 bytes".

The ~7KB is the size of all shaders in Resources/unity_builtin_extra. Unfortunately, the build report does not give us smaller granularity sizes for those shaders so I think we need to either better inform the user or hide those shaders to avoid confusion.

Sounds like a bug with the build report. Looking back at the Editor log after a build, I can see the problem. I think it's better to leave the information in than to hide these shaders (as with everything else in Project Auditor, let's err on the side of over-reporting rather than under-reporting!) and maybe file a bug/feature request with the build team. As a quick hack, do you think it makes sense to try to count all of the built-in/hidden shaders and divide the total size by the number of shaders so that each one can be displayed with the "average" size rather than the total size? Or maybe just to collapse them all down to a single entry called "Built-in Shaders (see Graphics settings)" or something?

Alternatively, what do you think about adding either: a right-end panel (similar to diagnostics Details/Recommendation) containing scrollable lists of keywords a small [...] button that opens a popup list of keywords, similar to the shader inspector one.

Either of these options sound good. I think a scrolling vertical list with each keyword on a new line is probably a better idea than what I suggested. Maybe even two panels, one for Keywords and the other for Platform Keywords.

mtrive commented 2 years ago

I think I am leaning for the right panels option:

variants-right-panel

mtrive commented 2 years ago

The shader size is indeed reported correctly in the AssetBundle build.

You mean in the editor log?

Is the AB build report where Project Auditor is finding out about the shader in the first place?

Yes

Or is it from an IPreprocessShaders callback before the build report is complete and the size is known?

The IPreprocessShaders callback gives you stage, pass, keywords, etc... but not the size

unitystevem commented 2 years ago

I think I am leaning for the right panels option:

Yes, this looks great. Do/can those panels scroll if there's too much content to fit into them?

You mean in the editor log?

Yes. The Editor log shows a build summary for the AssetBundles and then for the main build, and in the AB that contains the shader, it shows the size. Project Auditor picks up that the shader was built, but doesn't seem to retain the size. I guess there's a problem with matching up the data we get from IPreprocessShaders with what's in the Editor log. The build report only shows the last build, the executable itself, so it only shows .cs files in my test project.

mtrive commented 2 years ago

@unitystevem

Do/can those panels scroll if there's too much content to fit into them?

Now they do.

keywords-scroll

As far as size of shaders built into AssetBundles, I think we are going to explore a different solution based on AssetBundles analysis rather than the BuildReport. For the time being, those sizes are reported as Unknown.

unknown-shader-size

Note that internal shaders built into _unity_builtinextra are now also reported as Unknown. This is a BuildReport limitation which does not include the individual _unity_builtinextra asset sizes.

mtrive commented 2 years ago

One more addition to this PR: There is a new Shader Compiler Messages view that reports all compilation messages, which otherwise would not be visible apart from the Shader Inspector.

shader-compiler-messages

mtrive commented 2 years ago

two more changes based on feedback:

shader-compilation-messages