search

Garux / netradiant-custom

The open-source, cross-platform level editor for id Tech based games.
https://garux.github.io/NRC/
Other
299 stars 52 forks source link

fix q3map2 loadmodel pathsep crossplat #200

Closed Vorschreibung closed 1 day ago

Vorschreibung commented 1 week ago

Radiant does some canonicalization on file paths, however 'q3map2' seems to lack some.

For example on Linux loading a '.map' with a model path that involves Windows path separators ('\') will show and render them fine in Radiant, however 'q3map2' will then fail to load them.

This commit applies some path canonicalization on q3map2's LoadModel to remedy this specific issue.

Vorschreibung commented 1 week ago

Ready for review :blush:

Vorschreibung commented 1 week ago

Thanks for the review!

Is it optimal place to fix this? Possibly there are other uses of vfsLoadFile potentially providing wrong slashes? Can just fix there once and for everyone, slashes fixing is done there in most cases anyway

https://github.com/Garux/netradiant-custom/blob/3acacb884ea961ccc4eccf79530756f727671694/tools/quake3/common/vfs.cpp#L302

Yeah indeed, AFAIS AssModel *LoadModel doesn't go through vfsLoadFile atm. but instead uses Importer::ReadFile from libs/assimp.

I guess Importer::ReadFile could make use of vfsLoadFile ?

Garux commented 1 week ago

Thanks for the review!

Is it optimal place to fix this? Possibly there are other uses of vfsLoadFile potentially providing wrong slashes? Can just fix there once and for everyone, slashes fixing is done there in most cases anyway https://github.com/Garux/netradiant-custom/blob/3acacb884ea961ccc4eccf79530756f727671694/tools/quake3/common/vfs.cpp#L302

Yeah indeed, AFAIS AssModel *LoadModel doesn't go through vfsLoadFile atm. but instead uses Importer::ReadFile from libs/assimp.

I guess Importer::ReadFile could make use of vfsLoadFile ?

https://github.com/Garux/netradiant-custom/blob/3acacb884ea961ccc4eccf79530756f727671694/tools/quake3/q3map2/model.cpp#L119-L124

Vorschreibung commented 1 week ago

@Garux I have an alternative PR here: https://github.com/Garux/netradiant-custom/pull/202 - please let me know if you prefer this approach

Garux commented 1 week ago

@Garux I have an alternative PR here: #202 - please let me know if you prefer this approach

Without deep investigation i still like idea to fix this in vfsLoadFile more than hardcoding set of keys, which may go out of sync.

Garux commented 3 days ago

This https://github.com/Garux/netradiant-custom/commit/7499ba60ea112da01100b23a115d5205451c2731 shall fix it.

Vorschreibung commented 1 day ago

This 7499ba6 shall fix it.

It did! Thanks :heart_hands: