Skip to content

Proposal to support passing target version when cross compiling#18

Open
etinquis wants to merge 1 commit into
veldrid:masterfrom
etinquis:feature/setshaderversion
Open

Proposal to support passing target version when cross compiling#18
etinquis wants to merge 1 commit into
veldrid:masterfrom
etinquis:feature/setshaderversion

Conversation

@etinquis

Copy link
Copy Markdown
Contributor
  • adds TargetVersion to CrossCompileInfo so that the caller can control
    the target compilation version (if 0 is passed, the behavior is as
    before)
  • adds targetVersion param to SpirvCompilation.CompileCompute/CompileVertexFragment
  • adds
    SpirvCompilation.SetDefaultTargetVersionForCrossCompileTarget(CrossCompileTarget,
    uint version) to allow caller to set global default to avoid having to
    pass versions into every Compile call

While working on the PBR sample, I found that I ran into compilation errors due to the default MSL version being too low. This change allows me to set the MSL version as seen here.

* adds TargetVersion to CrossCompileInfo so that the caller can control
  the target compilation version (if 0 is passed, the behavior is as
  before)
* adds targetVersion param to SpirvCompilation.CompileCompute/CompileVertexFragment
* adds
  SpirvCompilation.SetDefaultTargetVersionForCrossCompileTarget(CrossCompileTarget,
  uint version) to allow caller to set global default to avoid having to
  pass versions into every Compile call
return (major * 10000) + (minor * 100) + patch;
}

private static uint GetDefaultTargetVersion(CrossCompileTarget target, bool isCompute)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Depending on where you think defaults should live, I can replace this with 0-by-default and let the cpp code handle all non-user-specified defaults.

byte[] vsBytes,
byte[] fsBytes,
CrossCompileTarget target,
uint targetVersion,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could also make this nullable to be clearer with 'unspecified' default.

/// <param name="minor">MSL minor version</param>
/// <param name="patch">MSL patch version</param>
/// <returns></returns>
public static uint MakeMSLVersion(uint major, uint minor = 0, uint patch = 0)

@etinquis etinquis Dec 23, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I didn't look to closely at the details of the various version formats, but I'm not really content with having this dangling off the API like this.

Would you rather see something more strongly typed like struct TargetVersion(uint major, uint minor, uint patch) exposed to callers in the public interface, and then uint GetTargetVersion(Target, TargetVersion) internally? Or target-specific version structs MslVersion(major, minor, patch), GlslVersion(major, minor), or what have you?

@TechPizzaDev

Copy link
Copy Markdown

Just wanted to chime in and say that veldrid-spirv translation generates shaders with different versions and causes Error linking GL program: L0001 Shader languages do not match on my Android. This PR may be a "solution" to this problem. My current solution is to just replace the lang version in the generated files to the highest version generated.

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.

2 participants