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

Adding minetest.clear_craft(recipe) to Lua modding API #4034

Closed
wants to merge 7 commits into from
Closed

Adding minetest.clear_craft(recipe) to Lua modding API #4034

wants to merge 7 commits into from

Conversation

Foghrye4
Copy link
Contributor

This function allow modders remove previously registered recipes either by output or by input and type (including "fuel" and "cooking"). Plus, removed recipes no longer visible via crafting guides, because they eliminated from registry.

@mtango688
Copy link
Contributor

This would take care of #2319 if I reckon this correctly.

@Foghrye4
Copy link
Contributor Author

@mtango688 yes it is.

@sofar
Copy link
Contributor

sofar commented Apr 25, 2016

It would be good to have the motivation for this change included in the commit message (not the PR).

Can you amend the commit and include something like:

"Removing craft recipes is useful for mods that are altering default crafts or eliminating easy ways of obtaining items."

Would just work.

@Foghrye4
Copy link
Contributor Author

@sofar github commits can not be edited.

@sofar
Copy link
Contributor

sofar commented Apr 25, 2016

Yes you can. Just push --force an amended commit.

std::vector<CraftDefinition*> new_vec_by_input;
for (std::vector<CraftDefinition*>::iterator i2 = unhashed_inputs_vec.begin();
i2 != unhashed_inputs_vec.end(); ++i2) {
if(def != *i2)
Copy link
Contributor

Choose a reason for hiding this comment

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

{ on same line and space between if and (

@Foghrye4
Copy link
Contributor Author

Foghrye4 commented Apr 26, 2016

@est31 edits was made according your notes.
Would you be so kind to add old labels to newly created pull request here: #4035
which was in this pull request here: #3962

  • Script API
  • Enhancement
  • Merge for testing candidate

?
They was so fancy-looking, so i miss them.

@est31
Copy link
Contributor

est31 commented Apr 27, 2016

lol

@Foghrye4
Copy link
Contributor Author

@est31 Thank you!

@sofar
Copy link
Contributor

sofar commented May 26, 2016

I was thinking this was merged already :(

@est31
Copy link
Contributor

est31 commented May 26, 2016

From what I can remember it looked mostly okay, but I didn't +1 because of the freeze.

@est31
Copy link
Contributor

est31 commented May 26, 2016

Giving it a "looks good" 👍 now.

for (std::vector<CraftDefinition*>::iterator i2 = unhashed_inputs_vec.begin();
i2 != unhashed_inputs_vec.end(); ++i2) {
if(def != *i2)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

if (condition)
        statement;

no braces if statement is only a single statement - space after if

@Foghrye4
Copy link
Contributor Author

@sofar extra spaces added to code. Thank you for checking code style for me.

i = unhashed_inputs_vec.size(); i > 0; i--) {
CraftDefinition *def = unhashed_inputs_vec[i - 1];
// If the input doesn't match the recipe definition, this recipe definition later will be added back in source map.
if (!def->check(input, gamedef)){
Copy link
Contributor

Choose a reason for hiding this comment

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

one more space

@sofar
Copy link
Contributor

sofar commented May 27, 2016

A few more missed style elements, plus it would be nice to get the reserve() comments in as well.

@Foghrye4
Copy link
Contributor Author

@sofar 'reserve()' is more or less obvious for those, who know C++ enough. Anyway, comments are added.

@sofar
Copy link
Contributor

sofar commented May 28, 2016

Ah, I just didn't see you added them before already - this looks fine.

@sofar
Copy link
Contributor

sofar commented May 28, 2016

I took the default planks recipe, and threw it in minetest.clear_craft().

https://github.com/minetest/minetest_game/blob/master/mods/default/crafting.lua#L3

minetest.clear_craft({
    output = 'default:wood 4'
})

throws an error: No crafting specified for output (output="default:wood 4")

minetest.clear_craft({
    recipe = {
        {'default:tree'},
    }
})

Throws an error: No crafting specified for input

Note that the documentation says that the formatting should be identical to the thing passed to register_craft, and I copy-pasted this from the actual calls to register_craft.

@mtango688
Copy link
Contributor

@sofar that sounds like a documentation error to me. IMO clear_craft really should make you specify both the output and the recipe.

@Foghrye4
Copy link
Contributor Author

@mtango688, @sofar Yes, it is my mistake in documentation.

minetest.clear_craft({
    recipe = {
        {'default:tree'},
    }
})

Seems work fine for me. Did you specify 'default' in 'depends.txt' of your test mod?

@sofar
Copy link
Contributor

sofar commented May 29, 2016

Ok, it works, I perhaps missed the dependency on default, which isn't entirely obvious. Perhaps the error message should be more clear -> "There is no recipe for this (input|output)" ?

@paramat
Copy link
Contributor

paramat commented Jun 24, 2016

^ @Foghrye4
@sofar how close are you to +1 this? Does it just need a better error message or are there other issues?

@Foghrye4
Copy link
Contributor Author

@paramat Yes, i saw sofars' question. And answer is: yes, perhaps.

@sofar
Copy link
Contributor

sofar commented Jun 25, 2016

👍 this is working well and great for subgames (and badly needed).

@paramat
Copy link
Contributor

paramat commented Jun 25, 2016

@est31 maybe you'd like to re-assess your +1 due to recent changes?

@paramat
Copy link
Contributor

paramat commented Jun 25, 2016

@Foghrye4 please can you fix the long lines? Max 90 columns with tab size = 4. Applies to comments too.

@Foghrye4
Copy link
Contributor Author

@paramat sure. Done.

@est31
Copy link
Contributor

est31 commented Jul 5, 2016

281e9f3

@est31
Copy link
Contributor

est31 commented Jul 5, 2016

okay, re-approved, and merged; see above.

@Foghrye4 Foghrye4 deleted the clear_craft branch July 6, 2016 16:15
@Foghrye4
Copy link
Contributor Author

Foghrye4 commented Jul 6, 2016

2 more to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants