gboisse / gfx

A minimalist and easy to use graphics API.
MIT License
498 stars 35 forks source link

Increase max name length #116

Closed VladimirMakeew closed 5 months ago

VladimirMakeew commented 6 months ago

Increasing kGfxConstant_MaxNameLength up to 260 characters (MAX_PATH from windows.h header file) should fix the problem with long shader paths. The root of the problem is GfxProgram::name buffer that is too small to store long paths and the full path to the shader file is truncated (for example in GfxInternal::createProgram).

gboisse commented 6 months ago

Thanks Vladimir, however this "name" should not be used in any shader compilation-related process. If it does, we should fix it.

The name of all gfx resources should only ever be used for debugging purposes (so it shows up in a Pix capture for instance) and as a matter of fact can be renamed by the user of the API to anything.

Under the hood, the full name and path should be stored as heap-allocated strings under the internal Program structure: https://github.com/gboisse/gfx/blob/7e89c1156aeb4dc8323cae5da343caa14fd980da/gfx.cpp#L645-L646

Let's make sure that these are used for all shader compilation functions and not that (re-namable) GfxProgram::name, which is, again, purely intended for debugging purposes and should not be used for anything else.

gboisse commented 6 months ago

Investigated this a bit further and I'd say this buffer is probably the culprit: https://github.com/gboisse/gfx/blob/7e89c1156aeb4dc8323cae5da343caa14fd980da/gfx.cpp#L8116

Please turn both shader_file and wshader_file into a std:string and std::wstring respectively and we can call it a day.

While you're at it, also apply a similar fix to the .pdb files please: https://github.com/gboisse/gfx/blob/7e89c1156aeb4dc8323cae5da343caa14fd980da/gfx.cpp#L8296

VladimirMakeew commented 6 months ago

Thanks for quick answer and investigation. I was thinking about this code: https://github.com/gboisse/gfx/blob/master/gfx.cpp#L2312 Instead of creating gfx_program.file_path from argument we use program.name. program.name as you mentioned is a small buffer for debug purposes, so I guess I should fix this place first.

gboisse commented 6 months ago

ha, good finding, you've definitely found another issue 😉

Yes, that code is incorrect indeed and needs fixing.

maoliver-amd commented 6 months ago

Investigated this a bit further and I'd say this buffer is probably the culprit:

https://github.com/gboisse/gfx/blob/7e89c1156aeb4dc8323cae5da343caa14fd980da/gfx.cpp#L8116

Please turn both shader_file and wshader_file into a std:string and std::wstring respectively and we can call it a day.

While you're at it, also apply a similar fix to the .pdb files please:

https://github.com/gboisse/gfx/blob/7e89c1156aeb4dc8323cae5da343caa14fd980da/gfx.cpp#L8296

Investigated this a bit further and I'd say this buffer is probably the culprit:

https://github.com/gboisse/gfx/blob/7e89c1156aeb4dc8323cae5da343caa14fd980da/gfx.cpp#L8116

Please turn both shader_file and wshader_file into a std:string and std::wstring respectively and we can call it a day.

While you're at it, also apply a similar fix to the .pdb files please:

https://github.com/gboisse/gfx/blob/7e89c1156aeb4dc8323cae5da343caa14fd980da/gfx.cpp#L8296

This has already been done in #114