Cherry Blossoms #3749

Merged
Ghost merged 15 commits from cherry_blossom into master 2023-06-27 05:58:33 +02:00
First-time contributor

This PR adds Cherry Blossoms and related wood set nodes. Thank you to @Wbjitscool and @SmokeyDope for the textures. Currently, there is no biome and no flowers, but the saplings can be obtained from dungeons and grown for the wood.

Testing

Obtain a sapling and grow it. Enjoy the cherry blossom trees. Make sure all of the cherry nodes (such as doors and fences) can be crafted and used.

This PR adds Cherry Blossoms and related wood set nodes. Thank you to @Wbjitscool and @SmokeyDope for the textures. Currently, there is no biome and no flowers, but the saplings can be obtained from dungeons and grown for the wood. ### Testing Obtain a sapling and grow it. Enjoy the cherry blossom trees. Make sure all of the cherry nodes (such as doors and fences) can be crafted and used.
Ghost added the
nodes
label 2023-05-20 20:29:54 +02:00
Member

No problem

No problem
ancientmarinerdev requested changes 2023-05-26 18:43:14 +02:00
ancientmarinerdev left a comment
Owner

Thanks for raising this PR. It's much appreciated. Apologies for the delay in reviewing this, i've had a hectic few weeks but it sure made me smile seeing this PR get raised.

It a very good starting point. I have added some comments, some of which are on cutting down duplication of code. Some of these have been done previously, so may seem frustrating, but it makes it all the more important to get started on improving these things, then we can roll it out to those other things.

There will inevitably be more trees, both internally and in mods, so we need something basic down to cut down on duplication and effort.

Thanks for raising this PR. It's much appreciated. Apologies for the delay in reviewing this, i've had a hectic few weeks but it sure made me smile seeing this PR get raised. It a very good starting point. I have added some comments, some of which are on cutting down duplication of code. Some of these have been done previously, so may seem frustrating, but it makes it all the more important to get started on improving these things, then we can roll it out to those other things. There will inevitably be more trees, both internally and in mods, so we need something basic down to cut down on duplication and effort.
@ -441,2 +440,2 @@
local names = { S("Oak Boat"), S("Spruce Boat"), S("Birch Boat"), S("Jungle Boat"), S("Acacia Boat"), S("Dark Oak Boat"), S("Obsidian Boat"), S("Mangrove Boat"), S("Oak Chest Boat"), S("Spruce Chest Boat"), S("Birch Chest Boat"), S("Jungle Chest Boat"), S("Acacia Chest Boat"), S("Dark Oak Chest Boat"), S("Mangrove Chest Boat") }
local craftstuffs = { "mcl_core:wood", "mcl_core:sprucewood", "mcl_core:birchwood", "mcl_core:junglewood", "mcl_core:acaciawood", "mcl_core:darkwood", "mcl_core:obsidian", "mcl_mangrove:mangrove_wood" }
local boat_ids = { "boat", "boat_spruce", "boat_birch", "boat_jungle", "boat_acacia", "boat_dark_oak", "boat_obsidian", "boat_mangrove", "boat_cherry", "chest_boat", "chest_boat_spruce", "chest_boat_birch", "chest_boat_jungle", "chest_boat_acacia", "chest_boat_dark_oak", "chest_boat_mangrove", "chest_boat_cherry" }
local names = { S("Oak Boat"), S("Spruce Boat"), S("Birch Boat"), S("Jungle Boat"), S("Acacia Boat"), S("Dark Oak Boat"), S("Obsidian Boat"), S("Mangrove Boat"), S("Cherry Boat"), S("Oak Chest Boat"), S("Spruce Chest Boat"), S("Birch Chest Boat"), S("Jungle Chest Boat"), S("Acacia Chest Boat"), S("Dark Oak Chest Boat"), S("Mangrove Chest Boat"), S("Cherry Chest Boat") }

We are changing translations? Did the keys change in the translation files also?

We are changing translations? Did the keys change in the translation files also?
Author
First-time contributor

No? That looks like one of the many quirks of git.

No? That looks like one of the many quirks of git.

Ah, you're right. Gitea has a really weird diff engine.

We will probably need to add in template for any new strings:

For example:

Cherry Chest Boat=
Cherry pressure plate=

etc.

Ah, you're right. Gitea has a really weird diff engine. We will probably need to add in template for any new strings: For example: Cherry Chest Boat= Cherry pressure plate= etc.
Author
First-time contributor

True

True
Ghost marked this conversation as resolved
@ -0,0 +8,4 @@
if mcl_core.check_growth_width(pos,7,8) then
minetest.set_node(pos, {name = "air"})
if r == 1 then
minetest.place_schematic({x = pos.x-2, y = pos.y, z = pos.z-2}, path, "random", nil, false)

These 3 need to be vector.offset()

These 3 need to be vector.offset()
Ghost marked this conversation as resolved
@ -0,0 +28,4 @@
})
local cherry_particle = {
velocity = vector.new(0,0,0),

vector.zero()

vector.zero()
Ghost marked this conversation as resolved
@ -0,0 +43,4 @@
action = function(pos, node)
minetest.after(math.random(0.1,1.5),function()
local pt = table.copy(cherry_particle)
pt.acceleration = vector.new(0,0,0)

vector.zero()

vector.zero()
Ghost marked this conversation as resolved
@ -0,0 +47,4 @@
pt.collisiondetection = false
pt.pos = vector.offset(pos,math.random(-0.5,0.5),-0.51,math.random(-0.5,0.5))
minetest.add_particle(pt)
pt.acceleration = vector.new(0,-1,0)

Because we're calling this a lot, maybe pull this out of the function so it's created once. Something like:

local one_down = vector.new(0,-1,0)

then inside:

pt.acceleration = one_down

Because we're calling this a lot, maybe pull this out of the function so it's created once. Something like: local one_down = vector.new(0,-1,0) then inside: pt.acceleration = one_down
Ghost marked this conversation as resolved
@ -0,0 +104,4 @@
return drop
end
local l_def = {

In core, we have the following, that all the main leaves use:

local function register_leaves

Can we not make this non-local, and access core as a dependency from this mod and just use this method. That way, nobody who calls it needs to know about orphans or define it. It's covered by the function. We then only pass in what differs rather than copying the full definition.

function mcl_core.register_leaves

Could we also do the same for:

register_tree_trunk
register_stripped_trunk
register_wooden_planks
register_sapling

Ideally, we should have done this for mangrove and potentially bamboo if possible, but if we can start now, at least we have a pattern we can use.

In core, we have the following, that all the main leaves use: local function register_leaves Can we not make this non-local, and access core as a dependency from this mod and just use this method. That way, nobody who calls it needs to know about orphans or define it. It's covered by the function. We then only pass in what differs rather than copying the full definition. function mcl_core.register_leaves Could we also do the same for: register_tree_trunk register_stripped_trunk register_wooden_planks register_sapling Ideally, we should have done this for mangrove and potentially bamboo if possible, but if we can start now, at least we have a pattern we can use.
Author
First-time contributor

wood api stuff good sir.

wood api stuff good sir.

It may not be as comprehensive as that, but it would a be a solid first step. After that is done, it gets a bit easier to see where things can be done clearer etc.

The scope of the wood api is a little large so breaking it down into smaller chunks could certainly help.

It may not be as comprehensive as that, but it would a be a solid first step. After that is done, it gets a bit easier to see where things can be done clearer etc. The scope of the wood api is a little large so breaking it down into smaller chunks could certainly help.
Ghost marked this conversation as resolved
@ -0,0 +189,4 @@
-- Door and Trapdoor
mcl_doors:register_door("mcl_cherry_blossom:cherrydoor", {
description = S("Cherry Door"),
_doc_items_longdesc = S("Wooden doors are 2-block high barriers which can be opened or closed by hand and by a redstone signal."),

Probably don't need these 2 lines because of this:


if not longdesc then
		if def.only_redstone_can_open then
			longdesc = S("This door is a 2-block high barrier which can be opened or closed by hand or by redstone power.")
		else
			longdesc = S("This door is a 2-block high barrier which can only be opened by redstone power, not by hand.")
		end
	end
	usagehelp = def._doc_items_usagehelp
	if not usagehelp then
		if def.only_redstone_can_open then
			usagehelp = S("To open or close this door, send a redstone signal to its bottom half.")
		else
			usagehelp = S("To open or close this door, rightclick it or send a redstone signal to its bottom half.")
		end
	end
Probably don't need these 2 lines because of this: ``` if not longdesc then if def.only_redstone_can_open then longdesc = S("This door is a 2-block high barrier which can be opened or closed by hand or by redstone power.") else longdesc = S("This door is a 2-block high barrier which can only be opened by redstone power, not by hand.") end end usagehelp = def._doc_items_usagehelp if not usagehelp then if def.only_redstone_can_open then usagehelp = S("To open or close this door, send a redstone signal to its bottom half.") else usagehelp = S("To open or close this door, rightclick it or send a redstone signal to its bottom half.") end end ```
Ghost marked this conversation as resolved
@ -0,0 +202,4 @@
mcl_doors:register_trapdoor("mcl_cherry_blossom:cherrytrapdoor", {
description = S("Cherry Trapdoor"),
_doc_items_longdesc = S("Wooden trapdoors are horizontal barriers which can be opened and closed by hand or a redstone signal. They occupy the upper or lower part of a block, depending on how they have been placed. When open, they can be climbed like a ladder."),

Probably don't need this because of:


	if not longdesc then
		if def.only_redstone_can_open then
			longdesc = S("Trapdoors are horizontal barriers which can be opened or closed and climbed like a ladder when open. They occupy the upper or lower part of a block, depending on how they have been placed. This trapdoor can only be opened or closed by redstone power.")
		else
			longdesc = S("Trapdoors are horizontal barriers which can be opened or closed and climbed like a ladder when open. They occupy the upper or lower part of a block, depending on how they have been placed. This trapdoor can be opened or closed by hand or redstone power.")
		end
	end
	usagehelp = def._doc_items_usagehelp
	if not usagehelp and not def.only_redstone_can_open then
		usagehelp = S("To open or close this trapdoor, rightclick it or send a redstone signal to it.")
	end
Probably don't need this because of: ``` if not longdesc then if def.only_redstone_can_open then longdesc = S("Trapdoors are horizontal barriers which can be opened or closed and climbed like a ladder when open. They occupy the upper or lower part of a block, depending on how they have been placed. This trapdoor can only be opened or closed by redstone power.") else longdesc = S("Trapdoors are horizontal barriers which can be opened or closed and climbed like a ladder when open. They occupy the upper or lower part of a block, depending on how they have been placed. This trapdoor can be opened or closed by hand or redstone power.") end end usagehelp = def._doc_items_usagehelp if not usagehelp and not def.only_redstone_can_open then usagehelp = S("To open or close this trapdoor, rightclick it or send a redstone signal to it.") end ```
Ghost marked this conversation as resolved
@ -0,0 +249,4 @@
mcl_sounds.node_sound_wood_defaults(),
{axey=1, material_wood=1},
nil,
S("A wooden pressure plate is a redstone component which supplies its surrounding blocks with redstone power while any movable object (including dropped items, players and mobs) rests on top of it."))

Is there a default we can use by not passing this in?

Is there a default we can use by not passing this in?
Ghost marked this conversation as resolved
@ -0,0 +260,4 @@
{material_wood=1,handy=1,axey=1},
1.5,
true,
S("A wooden button is a redstone component made out of wood which can be pushed to provide redstone power. When pushed, it powers adjacent redstone components for 1.5 seconds. Wooden buttons may also be pushed by arrows."),

Is there a default we can use by not passing this in?

Is there a default we can use by not passing this in?
Ghost marked this conversation as resolved

@PrairieWind Have you had chance to look into the feedback yet? Thanks.

@PrairieWind Have you had chance to look into the feedback yet? Thanks.
Author
First-time contributor

Looks like I need some more textures for the door -_-.

The top and bottom door textures.

Looks like I need some more textures for the door -_-. The top and bottom door textures.
Ghost force-pushed cherry_blossom from 4ec26cc56f to d2ef40bc2d 2023-06-17 18:02:17 +02:00 Compare
Author
First-time contributor

@Wbjitscool could you please make some top and bottom edge textures for the door? After the door update, it needs extra textures.

@Wbjitscool could you please make some top and bottom edge textures for the door? After the door update, it needs extra textures.

Haven't fully finished reviewing yet and haven't tested yet, but those changes look awesome. That nodes file looks really clean. It's a big step forward to making mcl2 more modable, so a big thank you from me, and I'm sure many modders would be very grateful.

Haven't fully finished reviewing yet and haven't tested yet, but those changes look awesome. That nodes file looks really clean. It's a big step forward to making mcl2 more modable, so a big thank you from me, and I'm sure many modders would be very grateful.
ancientmarinerdev reviewed 2023-06-18 20:07:15 +02:00
@ -0,0 +1,3 @@
title = mcl_cherry_blossom
author = PrairieWind

Do we need to add JoMW and Smokey to the credits?

Do we need to add JoMW and Smokey to the credits?
Author
First-time contributor

I can, as they did more work on it than I did, with the texture creation work.

I can, as they did more work on it than I did, with the texture creation work.
Ghost marked this conversation as resolved
Contributor

I just optimized the textures. They're now almost 94% smaller in size (mostly metadata). Check them out for anything that doesn't look right, and if they're fine, they're good to go in.

I just [optimized](https://git.minetest.land/MineClone2/MineClone2/src/branch/master/TEXTURES.md#further-optimization-with-optipng) the textures. They're now almost 94% smaller in size (mostly metadata). Check them out for anything that doesn't look right, and if they're fine, they're good to go in.

It looks great and the particle effect feels nice. I did find a few issues though:

Issue 1 (Fixed) - Name consistency is important to aid in api creation in future:

cherry_trapdoor rather than cherrytrapdoor
cherry_door rather than cherrydoor

Issue 2 (Fixed) - Cherry door, fence and gate cannot be crafted.

Issue 3 (Fixed) - Bonemeal. If a bonemeal a sapling, it will grow other trees but not that one. I placed down 6, bamboo'd 1, and 4 and 6 grew. See screenshot.

Issue 4 (Fixed) - Sapling and stick drop chances. Sapling drop chances feel quite low. I had about 7 trees and got 6 drops. It means you might not be able to grow sufficient cherry trees unless you make a profit from sapling drops. Not sure if this was bad luck or the drop rate is a little too low.

Issue 5 (Fixed) - Missing toppart and bottompart (you've already raised this, wanted to add it to list to make it easier to discuss things)

Issue 6 - Hidden wood in tree makes it hard to see any are left so tree can decay. Other tree's have wood next to other wood rather than diagonally.

It looks great and the particle effect feels nice. I did find a few issues though: Issue 1 (Fixed) - ~~Name consistency is important to aid in api creation in future:~~ ~~cherry_trapdoor rather than cherrytrapdoor cherry_door rather than cherrydoor~~ Issue 2 (Fixed) - ~~Cherry door, fence and gate cannot be crafted.~~ Issue 3 (Fixed) - ~~Bonemeal. If a bonemeal a sapling, it will grow other trees but not that one. I placed down 6, bamboo'd 1, and 4 and 6 grew. See screenshot.~~ Issue 4 (Fixed) - ~~Sapling and stick drop chances. Sapling drop chances feel quite low. I had about 7 trees and got 6 drops. It means you might not be able to grow sufficient cherry trees unless you make a profit from sapling drops. Not sure if this was bad luck or the drop rate is a little too low.~~ Issue 5 (Fixed) - ~~Missing toppart and bottompart (you've already raised this, wanted to add it to list to make it easier to discuss things)~~ Issue 6 - Hidden wood in tree makes it hard to see any are left so tree can decay. Other tree's have wood next to other wood rather than diagonally.
Author
First-time contributor

About the logs being hidden...that is sort of a thing from trying to replicate the real trees. Also not true, the big balloon oak trees have hidden wood too.

About the logs being hidden...that is sort of a thing from trying to replicate the real trees. Also not true, the big balloon oak trees have hidden wood too.

About the logs being hidden...that is sort of a thing from trying to replicate the real trees. Also not true, the big balloon oak trees have hidden wood too.

Thanks for implementing the changes and rapidly. You've nailed the big ones. That one with the hidden logs I dislike intently, but I can live with it. It ain't a dealbreaker. I've nudged smokey and jomw on Discord, and hopefully they can turn these textures around and we get this in!

> About the logs being hidden...that is sort of a thing from trying to replicate the real trees. Also not true, the big balloon oak trees have hidden wood too. Thanks for implementing the changes and rapidly. You've nailed the big ones. That one with the hidden logs I dislike intently, but I can live with it. It ain't a dealbreaker. I've nudged smokey and jomw on Discord, and hopefully they can turn these textures around and we get this in!
Author
First-time contributor

I mean I could remove some leaves, but I am not changing the wood pattern.

I mean I could remove some leaves, but I am not changing the wood pattern.
Member

here are the door texture topparts and side updates uses the Mineclone 2's cherry blossom textures I hope these textures work out well

here are the door texture topparts and side updates uses the Mineclone 2's cherry blossom textures I hope these textures work out well
Ghost requested review from ancientmarinerdev 2023-06-21 19:14:11 +02:00
Ghost force-pushed cherry_blossom from 4eecab8ac3 to 5733c0e3ac 2023-06-21 19:34:45 +02:00 Compare
ancientmarinerdev force-pushed cherry_blossom from 49b2714a65 to 3c4fb9abb2 2023-06-25 18:37:14 +02:00 Compare
ancientmarinerdev added 1 commit 2023-06-25 18:50:09 +02:00
ancientmarinerdev approved these changes 2023-06-25 18:55:00 +02:00
ancientmarinerdev left a comment
Owner

Looking great and testing well. I made a few changes @PrairieWind, so please verify I haven't messed anything up, but apart from that I'm happy to merge.

Looking great and testing well. I made a few changes @PrairieWind, so please verify I haven't messed anything up, but apart from that I'm happy to merge.
ancientmarinerdev added this to the 0.84.0 - Very Nice milestone 2023-06-25 18:55:58 +02:00
Ghost merged commit 70caacd369 into master 2023-06-27 05:58:33 +02:00
Ghost deleted branch cherry_blossom 2023-06-27 05:58:38 +02:00
Sign in to join this conversation.
No reviewers
No project
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: VoxeLibre/VoxeLibre#3749
No description provided.