Skip to content

Allow resource packs authors to acknowledge & dismiss shader warnings #2206

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

Merged
merged 5 commits into from
Jan 10, 2024

Conversation

Aeltumn
Copy link
Contributor

@Aeltumn Aeltumn commented Dec 12, 2023

This is a follow-up to recent changes where Sodium now shows warnings to users when they are using a resource pack that changes core terrain shaders. Some packs need to override these shaders for vanilla users (or use glsl files elsewhere) and could use some method to make the warnings go away for end users.

So, this PR adds a way for authors of resource packs to include additional information in the pack.mcmeta file which indicates that the author is OK with Sodium gracefully ignoring their shaders.

An example pack.mcmeta would look as follows:

{
    "pack": { "pack_format": 18, "description": "My resource pack with custom shaders" },
    "sodium": {
        "ignored_shaders": [
            "rendertype_solid.vsh",
            "rendertype_translucent.vsh",
            "rendertype_cutout_mipped.vsh",
            "fog.glsl",
            "light.glsl"
        ]
    }
}

No errors are logged specifically about the specific files mentioned (only an info message). This has the benefit that the author of a pack has to see the error messages first, and would see the error messages on any new shaders that get blacklisted in the future. However, the author now has the opportunity to fix their pack and then remove the warnings from the end users which do not care. The config specifies it's ignored shaders to hopefully inform the author that these shaders are not being run.

This is my first MR so let me know if I did anything wrong and I'll fix it asap!

(note I removed some old parts of this message as I realised I had wrongly assumed how Sodium handled the shaders beyond the warnings)

This gives pack authors a way to acknowledge and dismiss warnings so users do not get confused by them
@Octol1ttle
Copy link
Contributor

I opted to remove both the user warning and the console log for safe shaders. The message logged to the console only really states that you can't override the file and doesn't say much more. So, if a pack author already acknowledges the warnings the console warnings don't say anything relevant anymore. Could be re-added if the logs did have useful info.

I think it would be best to keep the console log message, but mention in the message that the resourcepack opted to have the warning hidden

@Aeltumn
Copy link
Contributor Author

Aeltumn commented Dec 12, 2023

I think it would be best to keep the console log message, but mention in the message that the resourcepack opted to have the warning hidden

That makes sense, I'll go change that.

@douira
Copy link
Collaborator

douira commented Dec 12, 2023

Does this mean these resource pack shaders correctly work with sodium or do they not work but also just not cause issues by being disabled?

@Aeltumn
Copy link
Contributor Author

Aeltumn commented Dec 12, 2023

Does this mean these resource pack shaders correctly work with sodium or do they not work but also just not cause issues by being disabled?

If a shader is marked as safe in the pack it is not warned about and not disabled. The assumption is that the pack author can ensure no issues are caused and use this to communciate that.

If the server wanted to disable a shader they could send a different pack I guess, or just not have it at all.

@douira
Copy link
Collaborator

douira commented Dec 12, 2023

ok I spoke to some people for information and want to clarify that even if the warning is disabled, sodium isn't loading any of the shaders that it replaces in vanilla from a resource pack, even if the warning has been disabled. Sodium isn't exposing it's internal shader format and isn't allowing replacement of sodium's internal terrain shaders, which differ from those vanilla uses. This PR only allows disabling the warning that a resource pack is attempting to replace core shaders which sodium already replaced. If a pack has the warning disabling flag that doesn't mean sodium will use the pack's shaders, only that it won't warn.

@Aeltumn
Copy link
Contributor Author

Aeltumn commented Dec 12, 2023

Talked about it on Discord, I wasn't aware that Sodium doesn't even use the base shaders, really sorry about that! I'll edit this PR in a bit to clarify that shaders are ignored, not safe, to clarify that they won't get used but the shader can be safely ignored.

Copy link
Collaborator

@douira douira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me

@jellysquid3 jellysquid3 added this to the Sodium 0.5.6 milestone Dec 16, 2023
Copy link
Member

@jellysquid3 jellysquid3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed change looks good to me, especially in regards to requiring resource pack authors to include the name of each file with a compatibility warning. There are just some code style nitpicks that should be resolved before we can merge it.

@jellysquid3 jellysquid3 added T-enhancement Type: Enhancement A-mods Area: Mod compatibility S-accepted Status: Request accepted but needs volunteer labels Dec 16, 2023
@Aeltumn Aeltumn requested a review from jellysquid3 December 16, 2023 14:32
@Aeltumn
Copy link
Contributor Author

Aeltumn commented Dec 16, 2023

Fixed the code style comments and re-tested everything. Should all be good now.

@jellysquid3 jellysquid3 added the P-high Priority: High label Dec 30, 2023
@jellysquid3
Copy link
Member

Sorry for the delay. I know this is probably annoying for some resource pack developers. We'll try to push a release with these changes as soon as possible.

@jellysquid3 jellysquid3 merged commit b67e6c2 into CaffeineMC:dev Jan 10, 2024
embeddedt referenced this pull request in FiniteReality/embeddium Jan 21, 2024
… (#2206)

This gives pack authors a way to acknowledge and dismiss warnings so users do not get confused by them.
IMS212 pushed a commit to IMS212/sodium-fabric that referenced this pull request Aug 6, 2024
…CaffeineMC#2206)

This gives pack authors a way to acknowledge and dismiss warnings so users do not get confused by them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mods Area: Mod compatibility P-high Priority: High S-accepted Status: Request accepted but needs volunteer T-enhancement Type: Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants