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

Farming: Make cotton look like cotton, add crafted string item #1876

Closed
wants to merge 1 commit into from
Closed

Conversation

paramat
Copy link
Contributor

@paramat paramat commented Aug 14, 2017

Addresses our oldest issue #404

Change farming:cotton item texture to look like cotton not string, texture by Napiphelios.
Add farming:string item crafted from farming:cotton.
Remove old alias minetest.register_alias("farming:string", "farming:cotton") it has been active for years and items tend to be converted faster than nodes. If anyone still has a stash of farming:string anywhere it is now supported as an item again.

Possible breakage:
Mods that use farming:cotton as a string item should now use farming:string, however the breakage is not severe as string is crafted from cotton 1 to 1 and is the same material, just the texture looks like a cotton ball now.
Any other possible breakage?
0.5 is a good chance to clean this mess up.

recipe = {
{"farming:cotton", "farming:cotton", "farming:cotton"},
}
})
Copy link
Member

Choose a reason for hiding this comment

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

Imho a vertical row would be better. 3 items of the same type in a horizontal row reminds to a slab recipe. But eg. technic also has a recipe to make a wire item vertical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm maybe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe 2 in a row so that 3 do not have to be crafted each time?

@cx384
Copy link
Contributor

cx384 commented Aug 14, 2017

I think you should need a spinning wheel to make string.

@paramat paramat removed the WIP label Aug 14, 2017
@paramat
Copy link
Contributor Author

paramat commented Aug 14, 2017

Tested.

@Desour
Copy link
Member

Desour commented Aug 14, 2017

@cx384 I think that would be too complex for MTG and there will be the same problems as for the renaming workbench.

@TumeniNodes
Copy link
Contributor

Spinning wheel would be a bit much (at least currently)
The current set up is clean and simple, and makes sense... and works.
I don't think it really matters which orientation the crafting calls for since it's not likely going to result in a string_slab .

@cx384
Copy link
Contributor

cx384 commented Aug 14, 2017

@DS-Minetest yeah, but a spinning wheel would fit in the medieval concept of minetest and it would enhance the Immersion.
Well, a wheel is circular ... but, imagine the spinning wheel as a craftitem which you need to craft string.

@TumeniNodes
Copy link
Contributor

TumeniNodes commented Aug 14, 2017

that could be done from another mod, which could also add even more uses and crafts for string.
honestly it would be better done that way, as an outside mod... and given time to grow and be tested. Who knows, maybe eventually added in another time but, for now a nice, simple move like this is perfect

@cx384
Copy link
Contributor

cx384 commented Aug 14, 2017

OK but without mods string doesn't have any use. Therefore there is no significant reason to add string to the minetest_game.

@TumeniNodes
Copy link
Contributor

TumeniNodes commented Aug 14, 2017

This could be considered the "first step" then.
This will get a very old issue out of the way, with a decent solution.
And "then" maybe ideas could come after for a use for string. Maybe such as adding bows to tools (for hunting) adding one bow to each tool materials?
But, as stated, getting this simple matter settled first is a good step.
(such simple matters with simple solutions really should not be so difficult)

@paramat
Copy link
Contributor Author

paramat commented Aug 15, 2017

Currently cotton is already string, so we're not adding string, many mods use the string item from MTG.

@TumeniNodes
Copy link
Contributor

I think @cx384 is reference to having uses for string within MTG, itself?
But that is a minor thing which could be discussed another time.
I feel this sort of fix should be merged a.s.a.p. to knock it out of the way. Tested and works fine.

IMO 2 MTG devs should be able to take something this trivial (but needed) and merge it without a week long (or longer discussion session) this is the sort of thing which causes drag and stalemate.
Gets put on hold or back-burner then they add up quick

@paramat
Copy link
Contributor Author

paramat commented Aug 15, 2017

Looking back, farming mod used to have cotton plants drop a farming:string item (with a string texture). More recently they drop farming:cotton (with a string texture) and there's an alias from farming:string to farming:cotton. So i don't see any serious breakage.

@TumeniNodes
Copy link
Contributor

TumeniNodes commented Aug 15, 2017

there is no breakage

@paramat
farming_cotton.png <-- optimized

Copy link
Contributor

@TumeniNodes TumeniNodes left a comment

Choose a reason for hiding this comment

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

optimized the new farming_cotton.png It is added to my latest comment edit

@paramat
Copy link
Contributor Author

paramat commented Aug 15, 2017

Thanks will use.

@paramat
Copy link
Contributor Author

paramat commented Aug 15, 2017

LOL it's down from 318 to 316 bytes, but will still use it.

@TumeniNodes
Copy link
Contributor

haha, I know but ... "optimized"
I'm going to start going through all the textures soon anyway, so this will be one more down

@paramat
Copy link
Contributor Author

paramat commented Aug 15, 2017

So optimisation has benefit even if there's no file size decrease? Maybe more efficient decompression?

@paramat
Copy link
Contributor Author

paramat commented Aug 15, 2017

Added your texture.

@TumeniNodes
Copy link
Contributor

It's still Napiophelios' texture, just 2bytes lighter in file size.
And tbh, that 2bytes does not make a difference and was done solely as this seems to be desired.
I figured I would nab this image while it was fresh in the pond but, tbh again, should be very careful with optimizing, it could end up doing more harm than good?

better a discussion elsewhere regarding that though at some point. there is a lot to the idea of using optimization (pros and cons) so it definitely needs more thinking/figuring, and I am definitely not a pro

@paramat
Copy link
Contributor Author

paramat commented Aug 17, 2017

I assume you use optipng? Optimisation does not, as far as i know, alter the visible pixels in any way, so i can't see any harm.
The only issue is with leaf nodes that need to retain a colour for the transapent pixels for 'opaque' mode. We have found those need to stay RGBA instead of indexed, and any opitmisation should be tested in 'opaque' leaf mode.

@TumeniNodes
Copy link
Contributor

yes, optipng. pngcrush seems overboard for what is needed here.
I will comment on a few things in Issues #1831 seems a more appropriate place for this topic

btw, this PR should already have approvals and merged (just sayin) tests fine, looks good. heh

@paramat
Copy link
Contributor Author

paramat commented Aug 17, 2017

Updated, 2 string returned from cotton above cotton, vertical used instead of horizontal.

@paramat
Copy link
Contributor Author

paramat commented Aug 18, 2017

Approval from SmallJoker (Krock) http://irc.minetest.net/minetest-dev/2017-08-18#i_5047098

@paramat
Copy link
Contributor Author

paramat commented Aug 18, 2017

08727bc

@paramat paramat closed this Aug 18, 2017
@paramat paramat deleted the cotton branch October 1, 2017 21:46
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

4 participants