ec- / Quake3e

Improved Quake III Arena engine
GNU General Public License v2.0
1.15k stars 148 forks source link

Shader Scripts Not Ordering Properly Across .shader Files #271

Closed WHS-Phoenix closed 3 months ago

WHS-Phoenix commented 3 months ago

If two shader scripts have the same name across different .shader files, they are not ordering properly. For example, the console background has red circuitry in normal Q3A. If playing Team Arena, this is supposed to be replaced by a grey static effect. This is not happening in Q3E, whereas it works properly in normal Q3A and IOQuake3. Specifics:

Launching Q3A normally (loading from baseq3): "console" shader in gfx.shader draws red circuit background Launching Q3A with +set fs_game missionpack: "console" shader in gfx2.shader should override the "console" shader in gfx.shader in baseq3 to draw the static effect instead.

This breaks irrespective of whether or not +set fs_game is involved. I copied the missionpack .pk3's into baseq3 and renamed them to load after pak0-pak9 and the same behavior persists.

ensiform commented 3 months ago

I could be wrong about this but the behavior is actually correct as something was fixed a very long time ago but I'm not sure the history is still there. The vanilla shader parser is actually broken in many ways and still partially in ioq3 but it's relied upon in that case so I'm not sure what the best course is.

ec- commented 3 months ago

If two shader scripts have the same name across different .shader files, they are not ordering properly

Unlike original Q3A/derivatives they are ordered properly: any later pk3 will override shaders with the same name, just like with other type of content (models, images, etc.) - and that's how modding system should work by design

ensiform commented 3 months ago

While I tend to agree what do we do about the already shipped assets that are like that in an official missionpack?

ec- commented 3 months ago

You can comment/fix legacy (-shipped with official release) shaders like that https://github.com/ec-/Quake3e/blob/master/code/renderer/tr_shader.c#L3477-L3496 There will be no revert to old (and fundamentally wrong) behavior

ensiform commented 3 months ago

How do you propose detecting the right gfx.shader when it exists in multiple pak#'s in baseq3 and it also exists in the copy of pak0 of missionpack plus in gfx2.shader so that it comments out the gfx.shader one if both are present?

Chomenor commented 3 months ago

The conflict is between gfx.shader and gfx2.shader, both in missionpack/pak0.pk3. In my filesystem project, I solved this by using the same precedence as q3e for shaders in different pk3s, but preserving the original precedence between shaders in the same pk3. This approach fixes this issue, I believe avoids the need for the tr_shader.c workarounds cited above, and also avoids potential issues with various other maps and mods.

This precedence trace from my filesystem shows how it uses the console from gfx2.shader due to the internal ordering of the pk3 file in order to match the original game behavior.

]/find_shader console
  ^3Element 1: ^7fs_basepath->missionpack/pak0.pk3->scripts/gfx2.shader->console

  ^3Element 2: ^7fs_basepath->missionpack/pak0.pk3->scripts/gfx.shader->console

  ^3Element 3: ^7fs_basepath->baseq3/pak5.pk3->scripts/gfx.shader->console

  ^3Element 4: ^7fs_basepath->baseq3/pak4.pk3->scripts/gfx.shader->console

  ^3Element 5: ^7fs_basepath->baseq3/pak0.pk3->scripts/gfx.shader->console

Use the 'fs_compare' command to get precedence details.
]/fs_compare 1 2
Check                           Result
------------------------------- ---------
resource_disabled               ---
special_shaders                 ---
server_pure_position            ---
basemod_or_current_mod_dir      ---
core_paks                       ---
current_map_pak                 ---
inactive_mod_dir                ---
downloads_folder                ---
shader_over_image               ---
shader_type                     ---
dll_over_qvm                    ---
direct_over_pk3                 ---
pk3_name_precedence             ---
extension_precedence            ---
source_dir_precedence           ---
intra_pk3_position              resource 1
intra_shaderfile_position       resource 1
case_match                      ---

Resource 1 was selected because it has a later position within the pk3 file than resource 2.
WHS-Phoenix commented 3 months ago

Unlike original Q3A/derivatives they are ordered properly: any later pk3 will override shaders with the same name, just like with other type of content (models, images, etc.) - and that's how modding system should work by design

You can comment/fix legacy (-shipped with official release) shaders like that https://github.com/ec-/Quake3e/blob/master/code/renderer/tr_shader.c#L3477-L3496 There will be no revert to old (and fundamentally wrong) behavior

So the "fix" make the code work "properly" actually breaks how things originally worked, which is how modders such as myself, map authors, and anyone else used to working with normal Q3A and other source ports are expecting the code to behave, and it's here to stay because "reasons". You do understand how this unnecessarily complicates things for the rest of us when there's inconsistencies like this between engines? It may be right on paper but it's a bad decision.

ensiform commented 3 months ago

I don't even see why they shipped gfx2 in the first place? They also shipped a copy of gfx which could have changed it. The other level detail shader is identical in both files and doesn't really need to exist in gfx2, plus nailtrail could easily have been in the modded gfx shader as well.

ec- commented 3 months ago

So the "fix" make the code work "properly" actually breaks how things originally worked

It can be claimed so to any fixed bug you're exploiting before and calling it 'work you broke for no reason' - I explained why it has been fixed, it also has been reported many, many years ago.

For example: in order to replace both shader and texture - you must provide 2 (TWO) pk3s: one with name preceeding base pak (to replace shader), and one with name succeeding base pak (to replace texture) - from technical view this is a pure cancer, not a "holy legacy" you should refer to or defend.

So there is an ability to add hacks for old/unsupported/abandoned mods, modern mods must account Q3e logic if they want to be played on Q3e (this includes hardened QVM where crappy mod code is not tolerated like in original Q3A etc.)

WHS-Phoenix commented 3 months ago

So there is an ability to add hacks for old/unsupported/abandoned mods, modern mods must account Q3e logic if they want to be played on Q3e

That's a pretty heavy-handed approach.... "Do it my way or else?" Consider what I have to do now. I have to either go back and "un-change" this behavior, implement someone else's file system that purportedly corrects this, or implement hacks that may not account for every map and/or PPM out there that might be affected by this... that's a lot of extra work for me to fix a problem that doesn't exist in other source ports, and not very good incentive for me to want to continue adapting Q3E for my own project when other ports such as IOQuake3 are not causing this sort of problem. I really don't want to drop Q3E support because there is a lot that I do like about your source port, especially the improvement to the lighting code, but I am also hesitant to continue because I must wonder how many other landmines like this might exist that I have not yet discovered? I realize you're committed to this but please understand this from the point of view of someone else working within the changes you've created.

Chomenor commented 3 months ago

There's really two separate issues here. What ec- appears to be talking about is changing the shader precedence between different pk3s to always match filesystem precedence (instead of being reversed in the absence of overlapping .shader filenames as it was originally). Personally I think this is very reasonable and usually not much of a compatibility issue.

The other issue is that, apparently as a side effect of how that change was implemented, the shader precedence within the same pk3 is now reversed in q3e compared to other engines. This is the cause of the console shader problem, does cause compatibility problems in general, and doesn't have any valid purpose that I know of.

I mentioned my filesystem as an example of an implementation that fixes this problem, but it can be fixed in q3e without implementing a new filesystem. There's probably a few different approaches, but one that comes to mind would adding an alternate version of ri.FS_ListFiles that reverses the order files within pk3s are returned, while keeping the same order of pk3s themselves. With some corresponding adjustments in the shader parsing code, it should be possible to achieve a result similar to my filesystem where precedence within pk3s (both same and different .shader file cases) matches the original game, and precedence between different pk3s matches the current q3e behavior.

WHS-Phoenix commented 3 months ago

The other issue is that, apparently as a side effect of how that change was implemented, the shader precedence within the same pk3 is now reversed in q3e compared to other engines. This is the cause of the console shader problem, does cause compatibility problems in general, and doesn't have any valid purpose that I know of.

Hmm... that definitely is food for thought. I think it's worth exploring.