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

Allow overriding tool capabilities through itemstack metadata #5612

Closed
wants to merge 2 commits into from

Conversation

Projects
@raymoo
Copy link
Contributor

raymoo commented Apr 18, 2017

This implements tool capability overriding for itemstacks, relevant to #36.

Some suggestions for review things that I am unsure of:

  • I used __tool_capabilities as an "internal" meta field to transmit/store tool capability overrides. The modder does not need to set this manually, tool capabilities are overridden with a separate itemstack metadata method.

The impact on mods is that the get_tool_capabilities lua API method on itemstacks is no longer equivalent to getting the tool capabilities from the item definition.

@raymoo

This comment has been minimized.

Copy link
Contributor Author

raymoo commented Apr 18, 2017

Demo mod: https://a.safe.moe/FOn1P.zip

This gives a special ability to wooden shovels: Right-clicking with a wooden shovel will permanently increase its digging speed immensely. There are also some basic tests for the Lua interface (but does not test everything).

@@ -87,6 +120,65 @@ void ToolCapabilities::deSerialize(std::istream &is)
}
}

void ToolCapabilities::serializeJson(std::ostream &os) const
{
Json::Value root(Json::objectValue);

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 19, 2017

Member

Json::objectValue not necessary

This comment has been minimized.

Copy link
@raymoo

raymoo Apr 19, 2017

Author Contributor

What do you mean? I thought Json::nullValue was the default.

This comment has been minimized.

Copy link
@raymoo

raymoo Apr 19, 2017

Author Contributor

Also is it unnecessary in all cases or just this one?

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 19, 2017

Member

in all cases it's not needed

{
Json::Value root;
is >> root;
if(root.isObject()) {

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 19, 2017

Member

missing space on every if

This comment has been minimized.

Copy link
@raymoo

raymoo Apr 19, 2017

Author Contributor

Fixed

@@ -108,15 +108,19 @@ struct ItemStack
}

// Get tool digging properties, or those of the hand if not a tool
const ToolCapabilities& getToolCapabilities(
ToolCapabilities getToolCapabilities(

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 19, 2017

Member

Having this as a copy is a huge drawback, this is a big object

This comment has been minimized.

Copy link
@raymoo

raymoo Apr 19, 2017

Author Contributor

@nerzhul I could give itemstack metas a shadow ToolCapabilities value that gets updated on deserialization and every set to the toolcaps key, would that be better?

This comment has been minimized.

Copy link
@raymoo

raymoo Apr 19, 2017

Author Contributor

And then I wouldn't need to copy because I could give a reference to that thing. Except I'm not sure if it could still be a const reference, how does that work?

EDIT: I guess my question is whether non-const reference can be safely (and implicitly) casted to const reference.

@raymoo

This comment has been minimized.

Copy link
Contributor Author

raymoo commented Apr 19, 2017

Added another commit because I wasn't sure about how it would affect the reviews. I will squash it when there are no outstanding review comments

@nerzhul

This comment has been minimized.

Copy link
Member

nerzhul commented Apr 19, 2017

@raymoo

This comment has been minimized.

Copy link
Contributor Author

raymoo commented Apr 19, 2017

That's what I asked about in the response to your review comment, I'll do that.

EDIT: Also realized my commit contained some extra members that I forgot to erase after I decided not to do this originally

@raymoo

This comment has been minimized.

Copy link
Contributor Author

raymoo commented Apr 19, 2017

@nerzhul I made a commit that keeps track of the current tool capabilities to allow making references and avoid redeserialization. I still don't understand when it is or isn't necessary to specify json types.

@nerzhul

This comment has been minimized.

Copy link
Member

nerzhul commented Apr 19, 2017

Oh just because it's not needed, i have a C++ server web application framework and i can tell you if you pass an object in constructor it initialize it with another objet type, instead of just instanciate a simple json object

return *cap;
const ToolCapabilities *def_cap =
itemdef->get(name).tool_capabilities;
if (def_cap != NULL) {

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 19, 2017

Member

braces are not needed

const ToolCapabilities& ItemStackMetadata::getToolCapabilities(
const ToolCapabilities &default_caps) const
{
if (toolcaps_overridden)

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 19, 2017

Member

you can inline it in header and use a nice ternary operator

@@ -87,6 +120,65 @@ void ToolCapabilities::deSerialize(std::istream &is)
}
}

void ToolCapabilities::serializeJson(std::ostream &os) const
{
Json::Value root(Json::objectValue);

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 19, 2017

Member

in all cases it's not needed

Json::Value groupcaps_object(Json::objectValue);
ToolGCMap::const_iterator gciter;
for (gciter = groupcaps.begin(); gciter != groupcaps.end(); ++gciter) {
Json::Value groupcap_object = gciter->second.toJson();

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 19, 2017

Member

unnedeed temp variable whcih trigger a huge memory copy

src/tool.h Outdated
@@ -47,6 +48,9 @@ struct ToolGroupCap
*time = i->second;
return true;
}

Json::Value toJson() const;

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 19, 2017

Member

don't return Json::Value instead use a Json::Value pointer as a param and make this function void

@raymoo raymoo force-pushed the raymoo:toolcap_override branch from 921465b to 73f3b80 Apr 19, 2017

@raymoo

This comment has been minimized.

Copy link
Contributor Author

raymoo commented Apr 19, 2017

Changes made, also squashed

Code quality is now good, someone should review the feature now

@nerzhul nerzhul added the Feature label Apr 19, 2017

@raymoo raymoo changed the title toolcap meta overriding Allow overriding tool capabilities through itemstack metadata Apr 19, 2017

@orwell96

This comment has been minimized.

Copy link
Contributor

orwell96 commented Apr 19, 2017

I see the following problem:
When adding a tool capabilities override to an item that has no tool-capabilities set, the override is ignored.
You should reorder the condition.

@raymoo

This comment has been minimized.

Copy link
Contributor Author

raymoo commented Apr 19, 2017

@orwell96 I did this on purpose so that it only works with items that are already tools. Do you think this should work with non-tools? Or maybe tools without an explicit tools capabilities table don't get one in the registry and that caused it to not work with something that should be a tool?

EDIT: Actually good catch, I thought that non-tools had their tool capabilities ignored, looks like they're not.

@raymoo raymoo force-pushed the raymoo:toolcap_override branch from 73f3b80 to 809afab Apr 19, 2017

@raymoo

This comment has been minimized.

Copy link
Contributor Author

raymoo commented Apr 19, 2017

Updated to address what @orwell96 mentioned.

@raymoo raymoo force-pushed the raymoo:toolcap_override branch from 809afab to 6d337bf Apr 19, 2017

@pandaro

This comment has been minimized.

Copy link
Contributor

pandaro commented Apr 21, 2017

i look at this magnificent PR, very useful!
I don't know if it's better to open a new issues, so i will ask here before.
The same concept can be done through a specific field in the player attribute back-end?
at the same manner:
Get the tool_capabilities from the player attribute back_end is not the same of the tool definition
Thanks

@raymoo

This comment has been minimized.

Copy link
Contributor Author

raymoo commented Apr 21, 2017

@pandaro Is the intended behavior that the player digs with the best of the tool's toolcaps and the player's toolcaps? If so, I think something similar could be achieved by applying the features from this PR to the player's hand override (see "Player Inventory Lists" in lua_api.txt)

EDIT: Also I think yes, it should be a separate issue.

@kaadmy

This comment has been minimized.

Copy link

kaadmy commented May 13, 2017

👍

@MisterXtreme

This comment has been minimized.

Copy link

MisterXtreme commented May 15, 2017

I would find this a very useful feature.
Just a couple of questions:
1 Could this instead be implemented through the tool properties?
2 It would be nice if this could also be applied to items other then tools, for example, so it could be used on armour, or the like?

@raymoo

This comment has been minimized.

Copy link
Contributor Author

raymoo commented May 15, 2017

@XtremeHacker

  1. What do you mean by the tool properties, and what would it mean to implement this through them?
  2. Currently this is only for tool capabilities.
@kaadmy

This comment has been minimized.

Copy link

kaadmy commented May 15, 2017

@raymoo can't ALL items, including held nodes etc. affect dig times? It isn't just for tools, I believe someone already mentioned this previously.

@raymoo

This comment has been minimized.

Copy link
Contributor Author

raymoo commented Jun 21, 2017

Rebased

@@ -3096,6 +3096,8 @@ Can be obtained via `item:get_meta()`.

#### Methods
* All methods in MetaDataRef
* `set_tool_capabilities([tool_capabilities])`: overrides the item's tool
capabilities to those provided, or unoverrides it if nil.

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Jul 10, 2017

Member

or uses the tool's original values if nil

This comment has been minimized.

Copy link
@raymoo

raymoo Jul 10, 2017

Author Contributor

Is uses the item definition's values if nil ok? I feel like it is clearer.

@@ -9,6 +9,22 @@
#define DESERIALIZE_KV_DELIM_STR "\x02"
#define DESERIALIZE_PAIR_DELIM_STR "\x03"

#define TOOLCAP_KEY "__tool_capabilities"

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Jul 10, 2017

Member

We have the special fields (empty), description and some for the coloring feature, but none of them with them underscores. How should this field be named?

This comment has been minimized.

Copy link
@raymoo

raymoo Jul 10, 2017

Author Contributor

My reasoning for naming it with an "internal"-looking name is that semantically the field isn't "just a string", it's a specifically formatted string and the field is mostly meant as an implementation detail of set_tool_capabilities. The other special fields are meant to be in a format that is directly settable from the metaref, so it makes more sense for the modder to deal with them directly, hence normal-looking names.

I'm not arguing that this is the only way to think about it, this is just a suggestion of a convention for meta fields that aren't meant to be set directly. Maybe it would be useful in another feature to disallow setting meta fields with a key with a special prefix.

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Sep 16, 2017

Member

This is a separate issue that needs considering, really. I suggest using "tool_capabilities" instead, and in another PR recommending that mods use the "modname:name" form for meta data.

@raymoo raymoo force-pushed the raymoo:toolcap_override branch from e58b64c to ab320a4 Jul 10, 2017

@MisterXtreme

This comment has been minimized.

Copy link

MisterXtreme commented Sep 8, 2017

Just wondering how this is going raymoo, I'm really hoping we can get thus feature to make dynamic tool abilities.

@raymoo

This comment has been minimized.

Copy link
Contributor Author

raymoo commented Sep 8, 2017

@MisterXtreme It's already complete, I just need to rebase. However, last time I did that, it stayed unreviewed until it had conflicts again. This time I am going to wait until a core dev shows interest.

@rubenwardy
Copy link
Member

rubenwardy left a comment

looks good, some small things

const ToolCapabilities *item_cap =
itemdef->get(name).tool_capabilities;
if (item_cap == NULL)
item_cap = itemdef->get("").tool_capabilities;

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Sep 16, 2017

Member

I'd add a comment saying: "fall back to the hand's tool capabilities"

@@ -9,6 +9,22 @@
#define DESERIALIZE_KV_DELIM_STR "\x02"
#define DESERIALIZE_PAIR_DELIM_STR "\x03"

#define TOOLCAP_KEY "__tool_capabilities"

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Sep 16, 2017

Member

This is a separate issue that needs considering, really. I suggest using "tool_capabilities" instead, and in another PR recommending that mods use the "modname:name" form for meta data.


// Overriddes
void clear();
bool setString(const std::string &name, const std::string &var);

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Sep 16, 2017

Member

use the override keyword, eg: void clear() override;

This comment has been minimized.

Copy link
@raymoo

raymoo Sep 16, 2017

Author Contributor

Doesn't this only work for virtual methods?

This comment has been minimized.

Copy link
@raymoo

raymoo Sep 16, 2017

Author Contributor

Oh, they are virtual.


Json::Value times_object;
std::unordered_map<int, float>::const_iterator timeiter;
for (timeiter = times.begin(); timeiter != times.end(); ++timeiter) {

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Sep 16, 2017

Member
for (auto time : times) {
    times_object[time->first] = time->second;
}

Json::Value groupcaps_object;
ToolGCMap::const_iterator gciter;
for (gciter = groupcaps.begin(); gciter != groupcaps.end(); ++gciter) {

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Sep 16, 2017

Member

use c++11 iterator, see above

toolcaps_overridden = true;
toolcaps_override = ToolCapabilities();
std::istringstream is(getString(TOOLCAP_KEY));
toolcaps_override.deserializeJson(is);

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Sep 16, 2017

Member

maybe add a comment like:

Will fails on the empty string, giving just ToolCapabilities()

This comment has been minimized.

Copy link
@raymoo

raymoo Sep 16, 2017

Author Contributor

If it's empty string, then wouldn't TOOLCAP_KEY not be contained?

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Sep 23, 2017

Member

Metadata::setString removes the field, so it seems to be okay.

This comment has been minimized.

Copy link
@RSL-Redstonier

RSL-Redstonier Jan 23, 2018

this is needed desperately for a tinkers construct clone I'm working on

{
Json::Value root;
is >> root;
if (root.isObject()) {

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Sep 16, 2017

Member

silently fails, should return false instead and then give a warning if the TOOLCAP_KEY string is non empty

This comment has been minimized.

Copy link
@raymoo

raymoo Sep 16, 2017

Author Contributor

Do you mean give a warning where deserializeJson is used? (I don't remember where)

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Sep 16, 2017

Member

Yeah, in updateToolCapabilities in itemstackmetadata. Or here is also fine actually, if you are able to get the length of IS (I forget if a stream could do this)

This comment has been minimized.

Copy link
@raymoo

raymoo Sep 16, 2017

Author Contributor

If I give a warning here, should I still return false?

This comment has been minimized.

Copy link
@raymoo

raymoo Sep 16, 2017

Author Contributor

If it's empty string, then wouldn't TOOLCAP_KEY not be contained?

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Sep 16, 2017

Member

actually, nevermind - there's no need to show a warning as set_tool_capacities will always be used

@raymoo raymoo force-pushed the raymoo:toolcap_override branch from ab320a4 to 0591509 Sep 16, 2017

@raymoo

This comment has been minimized.

Copy link
Contributor Author

raymoo commented Sep 16, 2017

Rebased and added support for custom hands, addressed @rubenwardy's comments, except I didn't add warnings.

@raymoo

This comment has been minimized.

Copy link
Contributor Author

raymoo commented Sep 17, 2017

Oh, I forgot to push

@raymoo

This comment has been minimized.

Copy link
Contributor Author

raymoo commented Sep 17, 2017

I mean commit

@raymoo raymoo force-pushed the raymoo:toolcap_override branch from 0591509 to 4953146 Sep 17, 2017

@raymoo

This comment has been minimized.

Copy link
Contributor Author

raymoo commented Sep 17, 2017

The build fail looks unrelated to my particular change, is that accurate?

EDIT: paramat told me on IRC that it's not related to my change.

? &hlist->getItem(0).getToolCapabilities(itemdef_manager)
: itemdef_manager->get("").tool_capabilities;

if (handToolcap != nullptr)

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Sep 23, 2017

Member

if (handToolcap) checks for NULL and nullptr: reduces the problem sources which could follow out of this.
getToolCapabilities uses NULL btw.

This comment has been minimized.

Copy link
@raymoo

raymoo Sep 23, 2017

Author Contributor

@SmallJoker NULL (aka 0) when cast to a pointer is the same as nullptr in C++.

This comment has been minimized.

Copy link
@raymoo

raymoo Sep 24, 2017

Author Contributor
@rubenwardy
Copy link
Member

rubenwardy left a comment

apart from this, lgtm

ItemStackMetadata() : toolcaps_overridden(false) {}

// Overriddes
virtual void clear() override;

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Sep 24, 2017

Member

no need for virtual with override. (virtual is only needed if this class was subclassed.) So just use override

see https://stackoverflow.com/a/39932616

This comment has been minimized.

Copy link
@raymoo

raymoo Sep 24, 2017

Author Contributor

Done

toolcap meta overriding
This makes it possible to modify the tool capabilities of individual itemstacks by
calling a method on itemstack metadata references

@raymoo raymoo force-pushed the raymoo:toolcap_override branch from 4953146 to 858e158 Sep 24, 2017

@nerzhul

This comment has been minimized.

Copy link
Member

nerzhul commented Sep 25, 2017

@rubenwardy

This comment has been minimized.

Copy link
Member

rubenwardy commented Sep 25, 2017

In c++11 override keyword replaces virtual

No, virtual declares a method to have a virtual table, which allows derived classes to be cast to their base class and the right method still called. Override is a compile time check that makes sure that a method overrides a method in one of its parent classes. You still need virtual on the base class when using override to be able to cast it to the base class. Before C++11 you didn't need to use either of those two keywords on a derived class' method.

@raymoo

This comment has been minimized.

Copy link
Contributor Author

raymoo commented Oct 2, 2017

@rubenwardy Does that lgtm mean an approval now that I've removed those keywords?

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Oct 2, 2017

Yes LGTM is an official approval.

@paramat paramat added the One approval label Oct 2, 2017

@SmallJoker
Copy link
Member

SmallJoker left a comment

Works

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Oct 23, 2017

Maybe retrigger builds as all failed to run?

@SmallJoker

This comment has been minimized.

Copy link
Member

SmallJoker commented Oct 23, 2017

@paramat See HEAD~1, where the main feature succeed. I tend to abort a TravisCI build if it affects the docs, as they're not checked anyway. The PR's conditions are met - it can be merged soon.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Oct 29, 2017

@paramat paramat closed this Oct 29, 2017

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.