shader-slang / slang

Making it easier to work with shaders
http://shader-slang.com
MIT License
2.18k stars 187 forks source link

IModule::getDependencyFilePath returns relative path for shaders in `cwd` #5332

Open tomas-davidovic opened 3 weeks ago

tomas-davidovic commented 3 weeks ago

For hot reload in SGL, we are pulling on all file dependencies of a slang module to find all the directories we need to watch for changes. The IModule::getDependencyFilePath gives up a bit of a mixed bag of results.

It returns:

Could you please shed some light on the expected behavior? From our side, the ideal behavior would be nullptr for modules with no file, and absolute filepath for all file based modules, but even a more detailed description of the possible outputs would help catch it all.

We do have a workaround, if the path is not absolute, we try to look for the returned string as a file in cwd, and if it is not there, we assume it is a string based module and skip, but we're not sure we have all the possible options covered.

csyonghe commented 3 weeks ago

In Slang, every module is associated with a file path, that file path can be a real physical path, or an imaginary one.

Slang cannot tell if a path is real or not. After all, when you load a module from source, you also get to specify the file path for the source that slang will use to identify this module. It makes sense for slang to return back whatever is being passed to the filepath parameter at loadModuleFromSource when you query file dependencies.

It is therefore the application's responsibility if they want to make sure a file path slang returns is an actual file.

csyonghe commented 3 weeks ago

In addtion, slang allows applications to pass in a custom IFileSystem interface to handle all file load/store requests. The path can be completely arbitrary/imaginary as long as the user provided file system handler can deal with them. There isn't a canonical way in Slang codebase to tell what accounts a valid path and what does not.

csyonghe commented 3 weeks ago

Because of the restricted view of the file system through the IFileSystem interface, it is also not possible for slang to canonical all paths into absolute paths. It is dependent on where the path comes from and slang itself has no knowledge to convert the path names.

csyonghe commented 3 weeks ago

I am closing this issue now, since I don't see any good actionable items. I think your workaround makes sense and feels the right way to address the issue.

tomas-davidovic commented 5 days ago

I am gonna ping this again, since I was digging a bit deeper after I did my workaround.

My problem comes from IncludeSystem::findFile. It first tries to find the file relative to the current path, and if it can, it just returns it like that. If it cannot, it will then try to prepend search paths to it, turning the path into absolute path.

In either case, it then goes and converts to absolute path for uniqueIdentity, but when asking for Module::getDependencyFilePath I get the "found path" which is "either relative or absolute, depending on the cwd" and not the uniqueIndentity, which is always absolute.

I don't think the result of foundPath should be change based on cwd. I'd just convert the path to absolute path always, even when found without using the search path (e.g., in STL I'd just call std::filesystem::absolute on the result). However, I do not know how is IncludeSystem related to the IFileSystem and how much could this change break the alternative implementations.