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 steel bar doors and trapdoors #2443

Conversation

@TumeniNodes
Copy link
Contributor

commented Aug 14, 2019

New PR for #2442

I decided to use default:tin_ingots in the recipes because,
it provides a simple solution to a recipe conflict with steel doors and steel trapdoors,
it is a legit process used to coat steel with tin to reduce corrosion (in cans)

citations:
>A "tin can" used to store food. Most cans used for this purpose are now tin-plated steel.

>So tin may be unassuming, but it's not unimportant. This metal is used to prevent corrosion
https://www.livescience.com/37355-tin.html

(I feel it is a perfect and viable solution and now xpanes recipes do not require any modification)
It also provides another use for tin, which now justifies the addition of tin even more.
Profit ;)

*Switched to just use default:steel_ingots per #2443 (comment)

I added a handle to the trapdoors to distinguish from xpanes:bar_flat (though these are not seen in the wield condition, only in inv. and ingame

I also took the liberty of making changes requested regarding indentations but, I went ahead and did the same for all of the existing doors' code

screenshot_20190814_002804
screenshot_20190813_012142

@DS-Minetest

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Ugh, the recipe seems so bad.
I don't know how tin coating is used exactly against corrosion, but in mtg there's never anything done against corrosion. And making steel (or iron?) bars out of some steel and tin seems unintuitive. And the xpanes bars and the doors should be made out of the same material.
I still want back the recipes using xpanes. Changing the shape would be weird and unintuitive for players. Also with xpanes the chances for recipe overlapping is lower.
(Btw. git isn't that difficult to use.)

@TumeniNodes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

I would like them to be made using the xpanes too but, I agree it seems overkill to add xpanes as a 2d hard dep for doors.
And I definitely do not feel it is a good idea to register these doors within xpanes itself.

I do not feel these new recipes are that bad, and they will become intuitive as players begin to use them.
It's not really that dramatic of a problem. At least I don't believe it is.

If core devs agree to allow xpanes to be justified as a 2d hard dep for these, I would be happy. Perhaps adding Iron bar doors as well might justify it?
Then again, I would need to try to add in the Iron ingots I have added to my own build, as well. ;)
But I am ok with this if they don't at the same time... it all works and makes sense the way it is.

And git can become a bit confusing in certain situations for novice users and/or individuals with memory issues (like myself)

@DS-Minetest

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Don't add a hard dep, add a soft optional one and only register the recipes if the mod is installed.

@TumeniNodes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

Don't add a hard dep, add a soft optional one and only register the recipes if the mod is installed.

hmmmm, tbh I hadn't even thought of that.
actually the doors themselves should only be reg if xpanes is loaded

I'll do that when I return from appt

@benrob0329

This comment has been minimized.

Copy link

commented Aug 14, 2019

Perhaps we should seperate the doors API from the door definitions themselves so that extra deps don't screw with games trying to be compatible.

@paramat paramat added the Feature label Aug 14, 2019
@paramat

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

I also took the liberty of making changes requested regarding indentations but, I went ahead and did the same for all of the existing doors' code

That's fine.

I'm not sure about using tin in the recipe, it's a clever solution but the material seems unsuitable.
I agree it's best to not alter the xpanes recipes.
I prefer these bar (trap)doors do not depend on xpanes in any way, even softly.
The main problem seems to be finding non-conflicting but intuitive recipes consisting of only steel material.

Separating APIs from registrations is possibly a good idea (if that doesn't break mods and games) but for a later PR.

@TumeniNodes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

Well if we use shaped recipes we can just move the steel ingots to the right for the bar doors and to the left for the solid steel doors?

Would have had to change the reg craft to shaped for all due to the API so I opted not to go this route.

@TumeniNodes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

New recipes added. Hope these are acceptable and do not conflict with any others out there

As a side note, if everyone would just play in creative, there would be no need for stupid recipes, I'm just sayin'

@Thomas--S

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

I'd suggest to add a steel stick (similiar to those of the darkage mod or the basic_materials mod) to Minetest Game.

It could be used as an ingredient for xpanes' steel bars as well as for the steel bar doors here.
Another advantage would be that mods won't have to define their own steel sticks anymore, but they can use Minetest Game's one, so the duplication of item definitions can be avoided more easily.

@paramat

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

The new recipes seem reasonable and similar to what i was considering.

@TumeniNodes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

I'd suggest to add a steel stick (similiar to those of the darkage mod or the basic_materials mod) to Minetest Game.

It could be used as an ingredient for xpanes' steel bars as well as for the steel bar doors here.
Another advantage would be that mods won't have to define their own steel sticks anymore, but they can use Minetest Game's one, so the duplication of item definitions can be avoided more easily.

I don't see this as a viable reason to add a steel stick (which I would call a steel_rod)
But to provide a material so 3rd party mods don't need to provide it for a specific use they have?
If this were done, then another similar step would be suggested and, where does it end?

The way to avoid duplication among 3rd party mods, is for those mods to make use of existing items from existing mods, if they require them.
But then we have a ton of 3rd party mods all dependent upon on another (a mess)
So sometimes duplication is unavoidable and ... "ok" ;)
(as long as those duplications are done in a way which do not cause a crash or other issues)

@TumeniNodes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

The new recipes seem reasonable and similar to what i was considering.

Wonderful. Does that warrant an approval? :D

@TumeniNodes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

@paramat I'm thinking of changing the recipe for the doors to use the outer two rows of the grid vertically instead, to kinda/sorta resemble making upright bars.
Thoughts on that? I think it makes a bit more sense.

{"x", "", "x"},
{"x", "", "x"},
{"x", "", "x"}

Then the trapdoors could use

{"", "", ""},
{"x", "", "x"},
{"x", "", "x"}
@Thomas--S

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

I can understand to some degree that you want to avoid new items.

However, I think that adding recipes like the ones suggested in this PR might cause some problems:
Firstly, they are not very intuitive because they have a different shape than all other (trap-)door recipes and secondly, the recipes will very likely break the recipes of mods where these shapes make more sense.

May I ask you to reconsider adding steel sticks (or however you wish to call them), please?
They would allow to use the recipes the players are already used to for crafting (trap-)doors.
I think they would also help quite a bit with avoiding duplicate recipes.

@TumeniNodes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

Honestly I do not oppose adding steel rods but that is up to core devs.
If it is agreed I will add them

If not, this can stand as is and with proper documentation the recipes for these doors will become intuitive over a short period of time

It is not that big of an issue to have a diff recipe layout for a new and specialized object
Actually it adds a nice variety

@paramat

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

I'm thinking of changing the recipe for the doors to use the outer two rows of the grid vertically instead, to kinda/sorta resemble making upright bars.
Thoughts on that? I think it makes a bit more sense.

Yes excellent, please do, more intuitive.
I haven't approved this PR, i have no objections but just feel very neutral, i expect other core devs will support it.

I don't think steel sticks/rods should be added, that's far worse than slightly weird recipes.

@Thomas--S

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

I don't think steel sticks/rods should be added, that's far worse than slightly weird recipes.

Please excuse me for asking again, but could you please explain this in a little bit more detail? I don't understand how adding the new item is "far worse than slightly weird recipes".

When we "block" rather common recipes with slightly weird ones, where we could easily use a more intuitive one, it will be very difficult for mods to find replacements for their "useful" recipes that rely on the same shapes.

@DS-Minetest

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

Imo, all (trap)doors recipes should base on the same shape and should only differ in the material they use.
I still can't understand why we can't have the recipe with the xpanes as materials (with optional dependencies). It would be by far the most intuitive.
The ⋮ ⋮ recipe however, wouldn't resemble a door, it's just two separate rods.

@TumeniNodes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

@SmallJoker
@sfan5
@rubenwardy
@Ezhh
@sofar
Would appreciate your thoughts, and reviews

@SmallJoker

This comment has been minimized.

Copy link
Member

commented Aug 18, 2019

My suggestions

  1. Optional-depend on xpanes
  2. Overwrite or duplicate xpanes:bar_flat as trapdoor (I'd prefer former)
  3. 3x Steel Bars + Steel Ingot for a locked steel bar door
@paramat

This comment has been minimized.

Copy link
Member

commented Aug 18, 2019

Here's an alternative i'm ok with:

Register the bar doors in xpanes mod, crafted from xpanes bars.
After all, registering items should ideally be done in a different place to the API for those items, this is something we're already considering.

Optionally registering these bar doors in doors mod but crafted from xpanes items and with an optional dependency on xpanes is a very messy implementation, i'm -1 for that.
SmallJoker's first 2 suggestions are messy too, i'm -1 for those.
The bar door (which we are making lockable) being 5 xpanes bars + a steel ingot (for the lock) is a good idea.

@TumeniNodes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2019

If an agreement can be made between at least two devs, I will make the adjustments agreed upon.
Until then I can't do anything.

I do feel using xpanes, as I originally had it, makes sense... now it just depends on "where" to register these doors, and the recipes.
I'm glad to do the work, but just need to know for certain which work needs to be done.

btw, new texture for the door itself (with lock)
doors_door_bar

and in-game
screenshot_20190818_160727

@DS-Minetest

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2019

2. Overwrite or duplicate `xpanes:bar_flat` as trapdoor (I'd prefer former)

What exactly do you mean with this? There is no such thing as xpanes:bar_flat.

@TumeniNodes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2019

xpanes:bar_flat is what they are called

https://github.com/minetest/minetest_game/blob/master/mods/xpanes/init.lua#L144

When using those panes in a recipe, you have to use "xpanes:bar_flat"
I found that out when I originally did this

@TumeniNodes TumeniNodes force-pushed the TumeniNodes:add_steel_bar_doors_and_trapdoors branch from 10e0438 to 5c1ceef Aug 19, 2019
@TumeniNodes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

rebased

@TumeniNodes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

@paramat I have another alternative to propose...
Register the new steel bar door and trapdoor in doors, with xpanes soft dep with this PR,
and when the time comes to separate doors reg from the api... then we address where to place the steel bar doors reg. (perhaps in xpanes as you suggest)

Does that sound reasonable?

@TumeniNodes TumeniNodes force-pushed the TumeniNodes:add_steel_bar_doors_and_trapdoors branch from 5c1ceef to 2b7f758 Aug 19, 2019
@paramat

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

Register the new steel bar door and trapdoor in doors, with xpanes soft dep with this PR,
and when the time comes to separate doors reg from the api... then we address where to place the steel bar doors reg. (perhaps in xpanes as you suggest)

I can't agree to that because it is doing something in a bad way only because it might (or actually might not) be put right later. It has to be right at all times.

I won't disapprove this PR as long as it is done the right way, but i would prefer these doors are provided by an optional mod instead of being in MTG.

@TumeniNodes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

I can reg them in xpanes, as Ive said I dont use crafting.
Ill pop the reg there and if it sits to long Ill just provide it as a mod, this hasnt gotten much notice or attention/interest anyway I just felt it would be nice to offer a diff door ingame

@sfan5 sfan5 added the Rebase needed label Sep 19, 2019
@sfan5

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

When rebasing, please also add S() for translations.

@TumeniNodes

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

will attempt to rebase shortly (it was throwing an error at me previously)

@TumeniNodes

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

sorry, I typed in a wrong character when rebasing and it caused a mess.
Closing this and the new PR is #2490

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