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 shape_type and base_material attributes to the shaped items definition #1594

Closed
wants to merge 2 commits into from
Closed

Add shape_type and base_material attributes to the shaped items definition #1594

wants to merge 2 commits into from

Conversation

bell07
Copy link
Contributor

@bell07 bell07 commented Feb 28, 2017

This PR introduces two item definition attributes placed for shaped items: base_material and shape_type. If the attributes are set the item becomes less value in inventory visibility that should animate mod developers to take more shape combinations.
See started discussion in https://forum.minetest.net/viewtopic.php?f=47&t=16788

I added a new tab "Shaped" to the creative inventory and filter them out from Nodes and Items. Unsure about the tools, that are all shaped too.

And I am sure there will be more usage purposes for the both fields if already exists.

@sofar
Copy link
Contributor

sofar commented Mar 8, 2017

Can this be a mod first? If this is successful as a mod (has lots of users, code works well and is debugged), and people really want this as part of minetest_game, then we can consider it.

For now, I would prefer to keep this outside of minetest_game.

suggestions: if you do this, I'd remove the default "blocks/nodes" tab and replace it with "materials", and then make a "shapes" tab. Or perhaps find a better way of distinguishing the two separate sets of nodes.

@bell07
Copy link
Contributor Author

bell07 commented Mar 8, 2017

I cannot see the way how the change can be moved to an other mod. The main change is in the first commit, adding 2 lines to mods default, doors, stairs and walls, to get the at definition time existing information accessible at runtime trough item definition. To get it separate means I need to start and maintain own minetest_game fork in competition to upstream (this way I do not like) or I need to do something hacky to get the both values in subsequent and then redefine the items distorting the mod_origin.

For the second commit you are right. I added the sfinv change just as proposal, to give something visible how the new attributes could be used. I do not use it, the primary usage is in smart_inventory. What do you think to merge the first commit only to the minetest_game? In this case the sfinv could be forked and enhanced to use the new information outside of minetest_game.

@sofar
Copy link
Contributor

sofar commented Mar 9, 2017

I'm against this patch as is. While I like the concept, I don't think it should be part of minetest_game currently. I feel it's much more interesting for a subgame with many more nodes than minetest_game, hence the question of whether this shouldn't be a mod instead.

I don't want to +1 this. If this is implemented as a proper mod (not a fork) and people actually use it, then I'll reconsider. I consider this the best way to prove that your code works and is useful to many people, and it's how I like to propose new changes myself as much as possible as well.

@bell07
Copy link
Contributor Author

bell07 commented Mar 9, 2017

Do you have a suggestion for me how I can add shape_type and base_material to the fences, gates, walls, trapdoors, stairs and slabs without forking the mods?

@sofar
Copy link
Contributor

sofar commented Mar 10, 2017

minetest.override_item()

@bell07
Copy link
Contributor Author

bell07 commented Mar 10, 2017

The issue is not to override the item but to detect which item needs the override and with which data. If there were a simple way to do it, the change in general were obsolete. The goal of the change is to provide this assembly information in simple way to other mods.
Stairs as the representative sample: They are defined trough function stairs.register_stair(). Inside this method the information is available: the result will be a "stair" and material is provided trough "recipeitem". Outside of this method the assignment to the both is lost. If the object of the change would be the 32 stairs only provided by "stairs" mod, the overrides could be trough a hardcoded list, but the stairs.register_stair() is an API implementation that is used by other mods like xdecor or castle that needs covered too.
Compare self: 2 lines change on API implementation versus a big maintenance-intensive mod (as workaround).

@sofar
Copy link
Contributor

sofar commented Mar 10, 2017

👎 for now. I don't feel mtg needs a drastic reshuffling of the creative inventory tabs.

@bell07 bell07 changed the title Special handling for shaped items in creative inventory Add shape_type and base_material attributes to the shaped items definition Mar 10, 2017
@bell07
Copy link
Contributor Author

bell07 commented Mar 10, 2017

Ok, I removed the controversial change on sfinv and changed the topic. Now the PR contains only the changes I need: Small enhancements on register_[shape_type] API implementations.
Agree this change is useless for the minetest_game, but the affected mods/API implementations are used in other subgames and mods too. This PR is made to minetest_game because it is the home of the API-mods that needs to be enhanced.

@sofar
Copy link
Contributor

sofar commented Mar 12, 2017

Even more 👎 right now - now you're making changes that have zero use in minetest_game...

Again, the best way to convince me is to prove that this is useful, and works well, entirely as a mod, first. Yes that may involve some manual lists of override_item calls.

@paramat
Copy link
Contributor

paramat commented Mar 12, 2017

👎 I agree wth sofar's comments.

@bell07
Copy link
Contributor Author

bell07 commented Mar 12, 2017

Ok, it is your choice. So you can close this PR unmerged.
Own mod with huge manual lists of items, that depends on other-mods implementations, to compensate missed small enhancement, is no way for me. So I will just propose to apply this patch to users that needs this feature. At the time smart_inventory mod solely does have usage for this attributes so this patch will be mentioned in his documentation.

fence, fencegate, stair, slab and wall items
…because the base_material is not available in old interface
@paramat paramat closed this Mar 13, 2017
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.

None yet

3 participants