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 grinding for pyrite from Everness mod #633

Conversation

gabriel1379
Copy link
Contributor

Hi,

I'm using Everness and wanted to be able to grind pyrite too, so created the option for it.
Submitting in case others might find it useful too.

…t if everness mod is present (also includes images for pyrite dust).
Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution.

  1. Strictly speaking, optional_depends is not necessary in this case. Recipes in technic can be registered out of order. minetest.get_modpath will also always work.
  2. Please run optipng -o7 -strip all or pngcrush on the two new textures. I think they could be smaller (thus less traffic to clients).

@gabriel1379
Copy link
Contributor Author

Thank you for the contribution.

1. Strictly speaking, `optional_depends` is not necessary in this case. Recipes in technic can be registered out of order. `minetest.get_modpath` will also always work.

2. Please run `optipng -o7 -strip all` or `pngcrush` on the two new textures. I think they could be smaller (thus less traffic to clients).

You're welcome.

Added the crushed .png-s. They didn't become much smaller, though.

As for the optional_depends, are you really sure that it's not needed? Because I've already had two mod conflict cases before that resulted in error and crash and the solution in both cases was just this. So I added it in in advance, to prevent any such issues for others.

I'm not as well versed in the programming intricacies of MineTest yet, though. What consequences does it exactly have it the optional_depends entry is there? Does it make anythnig slower or does it somehow degrade performance? In other words, what would be gained by removing it? Because if it doesn't actively hinder or hamper anything, then simply because of the aforementioned bug experiences I've had before, I think it would be a good idea to keep it.

But if you're really sure, then of course I'll remove it. As you like, just let me know.

@SmallJoker
Copy link
Member

SmallJoker commented Mar 4, 2024

What consequences does it exactly have it the optional_depends entry is there

It means that all mods in that list will be loaded before technic. This is important when trying to access API during the script init phase (i.e. before callbacks) or when there's a dependency on certain registrations. To my knowledge, there is no such requirement in technic.
By omitting the mod from optional_depends would allow everness to instead depend on technic if needed. It basically gives more freedom in the mod's dependency tree.
The trade-off from having short optional_depends lists is that the mod loading order is less defined, thus mods might load before or after - sometimes work - and sometimes don't if it turns out that there's indeed a dependency. This side-effect can only be tested reliably by adding this mod (technic) to the dependency list of the dependency (everness/mod.conf).

There is no performance difference. I'll leave the decision up to you whether to have more deterministic (but also more restricted) or mod dependencies or more freedom but uncertainty.

@gabriel1379
Copy link
Contributor Author

gabriel1379 commented Mar 4, 2024

Thanks for the detailed explanation, good to know. 👍
Well, generally I'm all for preventing potential bugs, especially ones I already know about, but I simply tried to see whether or not the game crashes if I remove it from the mod.conf . It didn't, so I pushed the change - it's now removed.

Edit: Also tested adding technic to the dependency list of everness. Worked that way too, so should be good.

@SmallJoker
Copy link
Member

Thank you for testing. Looks good. Will merge in a few days unless there are objections.

@gabriel1379
Copy link
Contributor Author

You're welcome. No objections from me. :)

@SmallJoker SmallJoker merged commit f47da0c into minetest-mods:master Mar 8, 2024
1 check passed
@gabriel1379 gabriel1379 deleted the feature/grind_pyrite_into_dust_with_everness_mod branch March 9, 2024 14:21
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

Successfully merging this pull request may close these issues.

2 participants