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

Emitted HLSL contains many unneeded new lines #57

Closed
jeremyong-az opened this issue Aug 25, 2022 · 5 comments
Closed

Emitted HLSL contains many unneeded new lines #57

jeremyong-az opened this issue Aug 25, 2022 · 5 comments

Comments

@jeremyong-az
Copy link
Contributor

// HLSL emission by AZSL Compiler 1.7.35 Win64
#line 14 "D:/o3de-atom-sampleviewer/user/AssetProcessorTemp/JobTemp-KiBTnM/StandardPBR_ForwardPass.azsl.dx12.prepend"


















static const float4 s_AzslDebugColor = float4 ( 16.0 / 255.0 , 124.0 / 255.0 , 16.0 / 255.0 , 1 ) ;
#line 11 "D:/o3de/Gems/Atom/Feature/Common/Assets/ShaderLib/Atom/Features/SrgSemantics.azsli"
#line 13 "D:/o3de/Gems/Atom/Feature/Common/Assets/ShaderResourceGroups/Decals/ViewSrg.azsli"














For example ^.

The new lines here corresponded to stripped comments, but instead of emitting new lines, we should just be adjusting the first argument of those #line directives.

@siliconvoodoo
Copy link
Contributor

siliconvoodoo commented Sep 6, 2022

Wouldn't it be better to preserve the comments? That would give some landmarks to authors who can navigate their code by familiarity, or leave anchors to search for. Especially in case of wrong line reporting, because of potential miss in the line directives (not unheard-of). I don't remember why we chose to remove them, it's possible that the reason is as simple as they just get skipped by AntlR's lexer, and we never made the effort of recovering them.

@jeremyong-az
Copy link
Contributor Author

jeremyong-az commented Sep 12, 2022

@siliconvoodoo There are two reasons I can think of for stripping comments.

  1. If we cache the preprocessed HLSL as an intermediate asset (something @santorac is prototyping), cached shader binaries would not be affected by either dead code (stripped via pound-def) or comments. If we strip unused functions from HLSL in the future, the comments associated with those functions would persist also.
  2. Really fat shaders sometimes don't work well with tools that do source analysis (e.g. Pix or Nsight), but I've noticed this has improved quite a bit in recent years

It may be useful to have an option to preserve them however.

@siliconvoodoo
Copy link
Contributor

fair!

btw I found the likely culprit for the empty lines, I suspect commit 396d653

Also I'm noting bugs in line directive generation (that we should be relying on instead of empty lines btw).
image
This might relate to bug #39

@siliconvoodoo
Copy link
Contributor

siliconvoodoo commented Sep 19, 2022

Ok I have a fix. (commit b3518ad)
Before:
image

In between (change to a line emission system rather than line feeds)
image

After improvement with "line economy" (only reemit when out of sync)
image

Example2:
image

@siliconvoodoo
Copy link
Contributor

Submitted in #61

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

No branches or pull requests

2 participants