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 give_initial_stuff API (and a setting) #579

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
7 participants
@rubenwardy
Copy link
Member

commented Jul 17, 2015

Setting: "initial_stuff".

give_initial_stuff.give(player)
-> Give initial stuff to "player"

give_initial_stuff.add(stack)
-> Add item to the initial stuff
-> Stack can be an ItemStack or a item name eg: "default:dirt 99"
-> Can be called after the game has loaded

give_initial_stuff.clear()
-> Removes all items from the initial stuff
-> Can be called after the game has loaded

give_initial_stuff.get_list()
-> returns list of item stacks

give_initial_stuff.set_list(list)
-> List of initial items with numeric indices.

give_initial_stuff.from_csv(str)
-> str is a comma separated

@rubenwardy rubenwardy force-pushed the rubenwardy:patch-1 branch from 96ae430 to e100984 Jul 17, 2015

@rubenwardy rubenwardy changed the title Add items table for give_initial_stuff and Add an API for give_initial_stuff Improve give_initial_stuff Jul 17, 2015

@rubenwardy rubenwardy changed the title Improve give_initial_stuff Improve give_initial_stuff (Add setting and an API) Jul 17, 2015

minetest.register_on_newplayer(function(player)
--print("on_newplayer")
local stuff_string = minetest.setting_get("initial_stuff") or
"default:pick_steel, default:axe_steel, default:shovel_steel," ..

This comment has been minimized.

Copy link
@est31

est31 Jul 17, 2015

Contributor

why do you have whitespaces between the itemstrings? Does it even work that way?

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Jul 17, 2015

Author Member

There's a trim function in give(). It's for user friendliness.

This comment has been minimized.

Copy link
@est31

est31 Jul 17, 2015

Contributor

ok then, I haven't seen the trim. User friendliness is a good goal.

player:get_inventory():add_item('main', 'default:shovel_steel')
player:get_inventory():add_item('main', 'default:cobble 99')
minetest.log("action",
"Giving initial stuff to player " .. player:get_player_name())

This comment has been minimized.

Copy link
@est31

est31 Jul 17, 2015

Contributor

One tab for indentation pls.

----------------------

give_initial_stuff.give(player)
^ gives to the player

This comment has been minimized.

Copy link
@est31

est31 Jul 17, 2015

Contributor

Perhaps this should document that it checks for the setting.

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Jul 17, 2015

Author Member

The function doesn't check the setting - the setting is checked at load time. But yes, it should be noted that it reads the stuff variable which is set from the setting.

This comment has been minimized.

Copy link
@est31

est31 Jul 17, 2015

Contributor

what? load time? you do if minetest.setting_getbool("give_initial_stuff") then, thats not loadtime for me. Btw. at this point I even agree to you checking the setting every time, and not cacheing it, because its requested only when new players join, which happens seldom enough.

@est31

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2015

See comments otherwise 👍 fwiw.

@asl97

This comment has been minimized.

Copy link

commented Jul 19, 2015

unless i am reading it wrong, isn't one of the reason for this is to allow other mods to add item to the list of items to be given?

if that is the case, is give_initial_stuff.stuff guarantee not to change (for at least a few version)?

someone might want to change the name (in the future) since, (imo), it isn't that great.

maybe providing an api for adding an item would make it more future proof.

@rubenwardy

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2015

Do you mean change the name of the mod? It's bad practice [citation needed] to make globals other than the name of the mod, and it doesn't belong in minetest.*

@asl97

This comment has been minimized.

Copy link

commented Jul 19, 2015

i mean the name of the variable give_initial_stuff.stuff, the stuff.stuff part just doesn't look that great.

@rubenwardy

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2015

This hasn't been merged yet, I can still change it to .items

@asl97

This comment has been minimized.

Copy link

commented Jul 19, 2015

someone could change it in the future, adding a simple wrapper would make it so that people wouldn't have to update their mods whenever someone decide to change it.

function give_initial_stuff.add_item(item)
    table.insert(give_initial_stuff.stuff, item)
end

also, iirc, It's bad practice to directly access another mod (internal) variable let alone edit it.

@rubenwardy

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2015

But yes, I will add an add() function as it is easier to read:

give_initial_stuff.add("mod:item")

than

table.insert(give_initial_stuff.items, "mod:item")

but I'm not going to make give_initial_stuff.items private.

@est31 est31 force-pushed the minetest:master branch from 39de13f to 7066a6a Aug 9, 2015

stuff = stuff_string:split(",")
}

function give_initial_stuff.give(player)

This comment has been minimized.

Copy link
@kaeza

kaeza Dec 19, 2015

Contributor

Does this really need to be public?

"Giving initial stuff to player " .. player:get_player_name())
local inv = player:get_inventory()
for _, item in ipairs(give_initial_stuff.stuff) do
inv:add_item("main", item:trim())

This comment has been minimized.

Copy link
@kaeza

kaeza Dec 19, 2015

Contributor

Suggestion: Use ItemStack(item) instead of item:trim() so it will be possible to give items with custom metadata or whatever else ItemStack supports.

@rubenwardy rubenwardy force-pushed the rubenwardy:patch-1 branch from e100984 to 541db83 Jan 2, 2016

@rubenwardy

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2016

Updated. Reviews please.

@rubenwardy rubenwardy changed the title Improve give_initial_stuff (Add setting and an API) Add give_initial_stuff API (and a setting) Jan 2, 2016

local inv = player:get_inventory()
for _, stack in ipairs(give_initial_stuff.items) do
inv:add_item("main", stack)
end

This comment has been minimized.

Copy link
@kilbith

kilbith Jan 2, 2016

Contributor

Replace this loop by inv:set_list("main", give_initial_stuff.items) (much more efficient).

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Jan 2, 2016

Author Member

My original thought was that it may be possible that this is not called when the inventory is empty. But I guess if a modder wanted to do that, they should make their own loop using give_initial_items:get_list()

@rubenwardy rubenwardy force-pushed the rubenwardy:patch-1 branch 2 times, most recently from 3438538 to 3b9e583 Jan 2, 2016

function give_initial_stuff.give(player)
minetest.log("action",
"Giving initial stuff to player " .. player:get_player_name())
player:get_inventory():set_list("main", give_initial_stuff.items)

This comment has been minimized.

Copy link
@asl97

asl97 Jan 2, 2016

the inventory might not be empty. efficiency's a bad reason for this cause it only run when new player joins.

This comment has been minimized.

Copy link
@kilbith

kilbith Jan 2, 2016

Contributor

It's initial stuff. If a modder mess around it, that's his problem.

@rubenwardy rubenwardy force-pushed the rubenwardy:patch-1 branch from 3b9e583 to d92b1b1 Jan 2, 2016

@rubenwardy

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2016

Can I have a core dev opinion?

@paramat

This comment has been minimized.

Copy link
Member

commented Feb 24, 2016

👍 Nice feature.

@paramat paramat added the One approval label Mar 2, 2016

@sofar

This comment has been minimized.

Copy link
Member

commented Mar 16, 2016

Don't see why this shouldn't get merged. @Ekdohibs @sfan5?

give_initial_stuff.items = {}
end

function give_initial_stuff.from_csv(str)

This comment has been minimized.

Copy link
@kaeza

kaeza Mar 17, 2016

Contributor

I think this should be renamed to add_from_csv. Just from_csv gives the impression it splits the values and returns something.

give_initial_stuff.items = list
end

function give_initial_stuff.get_list()

This comment has been minimized.

Copy link
@kaeza

kaeza Mar 17, 2016

Contributor

This function and the above are kinda redundant when you can just set give_initial_stuff.items = { } from another mod.

This comment has been minimized.

Copy link
@rubenwardy

give_initial_stuff.from_csv(str)
-> str is a comma separated list of initial stuff
-> sets items to that list

This comment has been minimized.

Copy link
@kaeza

kaeza Mar 17, 2016

Contributor

s/sets/adds/

Actually, "Adds items to the list of items to be given"

@paramat

This comment has been minimized.

Copy link
Member

commented Apr 29, 2016

@rubenwardy rubenwardy force-pushed the rubenwardy:patch-1 branch from d92b1b1 to b34071c Apr 29, 2016

@rubenwardy

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2016

Done

@paramat

This comment has been minimized.

@paramat

This comment has been minimized.

Copy link
Member

commented Apr 30, 2016

@rubenwardy Sorry, this had conflicts in game_api.txt with #1060 which has priority. We'll still merge this for release if rebased.

@rubenwardy

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2016

Ok

@rubenwardy

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2016

I can do that in roughly 15 hours, but not now

@paramat

This comment has been minimized.

Copy link
Member

commented May 1, 2016

No problem.

@rubenwardy rubenwardy force-pushed the rubenwardy:patch-1 branch from b34071c to a05d467 May 1, 2016

@rubenwardy

This comment has been minimized.

Copy link
Member Author

commented May 1, 2016

Rebased.

@paramat

This comment has been minimized.

Copy link
Member

commented May 1, 2016

@paramat paramat closed this May 1, 2016

@rubenwardy rubenwardy deleted the rubenwardy:patch-1 branch May 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.