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

D3DCompile bridge ignore include file handler #161

Closed
simontaylor81 opened this issue Mar 28, 2017 · 6 comments
Closed

D3DCompile bridge ignore include file handler #161

simontaylor81 opened this issue Mar 28, 2017 · 6 comments

Comments

@simontaylor81
Copy link

The compatibility bridge for D3DCompile (and friends) ignore the include file handler passed to them (i.e. ID3DInclude *pInclude passed to BridgeD3DCompile. See CompileFromBlob -- the pInclude parameter is unreferenced.

This means that we can't use this layer at all, because we rely on custom include file resolution to find our source files.

Is this a deliberate omission, or is it just something that hasn't been gotten to yet?

@marcelolr
Copy link
Contributor

This is deliberate but it's something we might get around to. The problem is that these don't map directly; the prior handler required the implementation to track pairs of Open/Close to build up a the include chain to resolve relative references (and also to track the system vs. local include types).

The new interface is easier to use - the compiler itself enforces rules around system vs. local includes and handles probing for files in the right order; the implementation simply needs to know to deal with the file not being found. It will also deal with absolute paths as much as possible.

This allows a pretty trivial implementation over an actual file system or a synthesized set of files. It does mean however that it's hard to implement the new interface over a prior implementation.

There are a few solutions we could attempt. If you can change your include handler, the easiest thing to do would be to have that implement the new interface as well, and we can change our bridge to QI for that and use it if available.

If you can't change the implementation of your include handler, it gets messier, as the right strategy will likely be couple to the implementation's behavior and, for example, whether it's resilient to missed files (or whether it goes into an error state and becomes unusable after a failed probe, for example).

@simontaylor81
Copy link
Author

Cool, thanks for the info -- that does sound a lot nicer! I am able to modify the include handler, so I'll probably write a new bridge method that just passes through the new interface directly but leaves the other stuff intact.

(In time we'll want to move over to the new interface completely, this is just a temporary thing to get us up and running quickly).

@marcelolr
Copy link
Contributor

@simontaylor81, I have a patch to handle the use of D3D_COMPILE_STANDARD_FILE_INCLUDE, but unfortunately my prior suggestion cannot be implemented - ID3DInclude doesn't inherit from IUnknown as far as I can tell (d3dcommon.h has it declared with DECLARE_INTERFACE with no base type). I think that's as far as we'll be able to go with a generic bridge.

@simontaylor81
Copy link
Author

Ok cool, thanks for the update. I've already switched to using the new interface anyway -- it's much nicer in general for various reasons so I'm definitely not complaining!

Maybe this limitation should be documented somewhere (if it isn't already)? Otherwise I'm happy to close this issue if you are.

@marcelolr
Copy link
Contributor

@simontaylor81 I've included a short version of this discussion over at https://github.com/Microsoft/DirectXShaderCompiler/wiki/D3DCompiler-DXC-Bridge. Please feel free to close the issue, and thanks!

@marcelolr
Copy link
Contributor

In the interest of historical cross-referencing, the issue thread #166 has a good discussion on the topic of include handler design.

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