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

Shader Scripts Not Ordering Properly Across .shader Files #271

Closed
WHS-Phoenix opened this issue Mar 3, 2024 · 12 comments
Closed

Shader Scripts Not Ordering Properly Across .shader Files #271

WHS-Phoenix opened this issue Mar 3, 2024 · 12 comments

Comments

@WHS-Phoenix
Copy link

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
Copy link
Contributor

ensiform commented Mar 3, 2024

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-
Copy link
Owner

ec- commented Mar 3, 2024

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
Copy link
Contributor

ensiform commented Mar 3, 2024

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

@ec-
Copy link
Owner

ec- commented Mar 3, 2024

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
Copy link
Contributor

ensiform commented Mar 3, 2024

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
Copy link
Contributor

Chomenor commented Mar 3, 2024

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
Copy link
Author

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
Copy link
Contributor

ensiform commented Mar 4, 2024

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-
Copy link
Owner

ec- commented Mar 4, 2024

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.)

@ec- ec- closed this as not planned Won't fix, can't repro, duplicate, stale Mar 4, 2024
@WHS-Phoenix
Copy link
Author

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
Copy link
Contributor

Chomenor commented Mar 5, 2024

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
Copy link
Author

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.

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

4 participants