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

Add missing null handling to GroupResourcePack #342

Open
wants to merge 3 commits into
base: 1.20
Choose a base branch
from

Conversation

soir20
Copy link

@soir20 soir20 commented Sep 1, 2023

Currently, two methods in GroupResourcePack will throw NullPointerExceptions if there is no pack for the given namespace. I've tweaked these methods so that the GroupResourcePack won't crash the game in these circumstances.

This isn't an issue when no other mods are present because vanilla also scans packs by namespace. However, I develop a mod that originally listed resources in all packs, regardless of whether they contained a particular namespace, which caused the game to crash.

There's an existing null check in open(); this PR makes null handling consistent throughout the whole GroupResourcePack.

@LambdAurora
Copy link
Contributor

I'm curious to see how you're listing those resources, in theory NPE should never be thrown if resources are interacted with correctly.

@soir20
Copy link
Author

soir20 commented Sep 1, 2023

My mod adds a resource pack that scans other packs for certain resources. I streamResourcePacks() from the ResourceManager and query resources from them directly to prevent infinite recursion and to query root resources.

Specifically, I was calling listResources() directly on the resource packs, rather than going through the ResourceManager. This has since been fixed to check that the pack has the given namespace, so there's no longer a crash. However, I don't think Quilt should crash in these circumstances because vanilla packs do not; there should at least be a descriptive exception if this is intended.

@LambdAurora LambdAurora added bug Something isn't working library: core Related to the core library. t: refactor This proposes a refactor. s: tested This pull request has been tested and confirmed as working. labels Nov 14, 2023
Comment on lines +141 to +142
// Iterating backwards as higher-priority packs are placed at the end.
for (var pack : packs) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this comment actually relevant ? It seems copy-pasted from above

Copy link
Author

Choose a reason for hiding this comment

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

It's the same for statement, just moved under the null check, so the comment is still relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working library: core Related to the core library. s: tested This pull request has been tested and confirmed as working. t: refactor This proposes a refactor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants