VoxeLibre/VoxeLibre
VoxeLibre
/
VoxeLibre
60
133
Fork 177
Code Issues 977 Pull Requests 89 Projects 4 Releases Wiki Activity

fix incorrect digtypes and missing sounds for some nether blocks #3351

Merged
ancientmarinerdev merged 25 commits from SmokeyDope/MineClone2:master into master 2023-02-04 01:27:50 +01:00
Conversation 11 Commits 25 Files Changed 4 +76 -29
SmokeyDope commented 2023-01-23 01:21:15 +01:00
Member
Copy Link

This pull request directly fixes two bug reports I made:

MineClone2/MineClone2#3350 Hoes should be the effective tool for warped wart blocks instead of swords

MineClone2/MineClone2#3347 Warped wart blocks make no sound

And another bug report made earlier:

MineClone2/MineClone2#3287 mcl_blackstone blocks cannot be mined with wooden pickaxe

Fixing tool digging inconsistencies

Some nether blocks were not properly mineable with when using what should be the right tools. This was due to typos and inconsistent digtype values across the nether blocks.

I fixed these bugs by correcting the typos, standardizing the digtypes so that hoes and swords are effective for wart blocks and shroomlight blocks, also adjusting the digtype values of the pickaxey mcl_blackstone blocks to conform to vanilla minecrafts standards by making them mineable with a wooden pick.

I lowered the hardness of the warped wart block and shroomlight block from 2 to 1 to match nether wart block's hardness.

Fixing missing sounds

Some nether blocks and plants were missing sounds when placed, broken, and walked on. For some of them this was due to the blocks node registers missing the 'sounds =' flag.

For some node registers with special 'on_rightclick = function' they were missing a 'call sound' function in the logic that should go after sucessful placement of the block.

I fixed these bugs by adding in the sound flags and functions needed to get sound working for the blocks.

!!!NOTE!!! Some plantlike nodes such as fungus and nether sprouts are still missing placement sound when directly placed on a block for reasons ive yet to understand. For example the nether vines wont make a placement sound when placed on top of a regular block, but will make a placement sound when placed on another vine.

Nether vine logic & missing placement checks

I added in some nether vine breaking code so that breaking one vine breaks the above vines for twisted and below vines for weeping.

Most nether plants are missing important chunks of placement checks most eggregiously with the nether vines. Adding in these placement checks would most likely fix the bug that lets the nether vines break bedrock as they grow. Many players say that they enjoy this unintended function of the vines and I feel that it makes sense to turn this bug into a feature. Maybe there can be a special bedrock check that lets the vines still break bedrock specifically.

Ancient debris touchups

Ancient debris is now smeltable in the blast furnace, netherite ingot crafting recipe is now shapeless.

Testing

place the blocks mentioned and see if they make placement sounds. Break them with the proper tool to see if it digs correctly and consistently across wart block types.

This pull request directly fixes two bug reports I made: https://git.minetest.land/MineClone2/MineClone2/issues/3350 Hoes should be the effective tool for warped wart blocks instead of swords https://git.minetest.land/MineClone2/MineClone2/issues/3347 Warped wart blocks make no sound And another bug report made earlier: https://git.minetest.land/MineClone2/MineClone2/issues/3287 mcl_blackstone blocks cannot be mined with wooden pickaxe ### Fixing tool digging inconsistencies Some nether blocks were not properly mineable with when using what should be the right tools. This was due to typos and inconsistent digtype values across the nether blocks. I fixed these bugs by correcting the typos, standardizing the digtypes so that hoes and swords are effective for wart blocks and shroomlight blocks, also adjusting the digtype values of the pickaxey mcl_blackstone blocks to conform to vanilla minecrafts standards by making them mineable with a wooden pick. I lowered the hardness of the warped wart block and shroomlight block from 2 to 1 to match nether wart block's hardness. ### Fixing missing sounds Some nether blocks and plants were missing sounds when placed, broken, and walked on. For some of them this was due to the blocks node registers missing the 'sounds =' flag. For some node registers with special 'on_rightclick = function' they were missing a 'call sound' function in the logic that should go after sucessful placement of the block. I fixed these bugs by adding in the sound flags and functions needed to get sound working for the blocks. !!!NOTE!!! Some plantlike nodes such as fungus and nether sprouts are still missing placement sound when directly placed on a block for reasons ive yet to understand. For example the nether vines wont make a placement sound when placed on top of a regular block, but *will* make a placement sound when placed on another vine. ### Nether vine logic & missing placement checks I added in some nether vine breaking code so that breaking one vine breaks the above vines for twisted and below vines for weeping. Most nether plants are missing important chunks of placement checks most eggregiously with the nether vines. Adding in these placement checks would most likely fix the bug that lets the nether vines break bedrock as they grow. Many players say that they enjoy this unintended function of the vines and I feel that it makes sense to turn this bug into a feature. Maybe there can be a special bedrock check that lets the vines still break bedrock specifically. ### Ancient debris touchups Ancient debris is now smeltable in the blast furnace, netherite ingot crafting recipe is now shapeless. ### Testing place the blocks mentioned and see if they make placement sounds. Break them with the proper tool to see if it digs correctly and consistently across wart block types.
👍 1 🚀 1
SmokeyDope added 2 commits 2023-01-23 01:21:19 +01:00
SmokeyDope added 1 commit 2023-01-23 02:29:01 +01:00
SmokeyDope added 1 commit 2023-01-23 03:05:04 +01:00
SmokeyDope changed title from WIP: fix digtypes for nether wart blocks, warped wart blocks, and shroomlight blocks to WIP: fix incorrect digtypes and missing sounds for some nether blocks 2023-01-23 04:06:11 +01:00
SmokeyDope added 1 commit 2023-01-23 17:55:23 +01:00
Ghost commented 2023-01-23 21:52:41 +01:00
First-time contributor
Copy Link

This PR Fixes #3350
This PR Fixes #3347

This PR Fixes #3350 This PR Fixes #3347
SmokeyDope added 1 commit 2023-01-23 22:30:37 +01:00
SmokeyDope added 1 commit 2023-01-23 23:17:24 +01:00
SmokeyDope commented 2023-01-23 23:18:47 +01:00
Author
Member
Copy Link

This PR Fixes #3287

This PR Fixes #3287
Ghost commented 2023-01-23 23:20:59 +01:00
First-time contributor
Copy Link

@SmokeyDope I think the hitbox size was commented out for a reason. I know it was commented out in a recent commit that I approved personally. Maybe look at a warped fungus without any texture packs with your change and you'll see why.

@SmokeyDope I think the hitbox size was commented out for a reason. I know it was commented out in a recent commit that I approved personally. Maybe look at a warped fungus without any texture packs with your change and you'll see why.
❤️ 1
SmokeyDope commented 2023-01-23 23:29:13 +01:00
Author
Member
Copy Link

@PrairieWind Good catch, I see what you mean

@PrairieWind Good catch, I see what you mean
Screenshot from 2023-01-23 17-27-16.png
474 KiB
Screenshot from 2023-01-23 17-27-16.png
❤️ 1
SmokeyDope added 1 commit 2023-01-23 23:34:34 +01:00
SmokeyDope added 1 commit 2023-01-24 17:24:45 +01:00
SmokeyDope added 1 commit 2023-01-24 18:15:30 +01:00
6f22d7daa4 Add node breaking logic to twisting vines and weeping vines 
breaking twisting vines will break the ones above them, breaking weeping vines breaks the ones below them
SmokeyDope added 1 commit 2023-01-25 05:07:07 +01:00
SmokeyDope added 1 commit 2023-01-25 06:47:47 +01:00
SmokeyDope added 1 commit 2023-01-30 06:03:57 +01:00
SmokeyDope added 1 commit 2023-01-30 06:38:40 +01:00
Michieal commented 2023-01-30 08:41:54 +01:00
Member
Copy Link

@SmokeyDope I think the hitbox size was commented out for a reason. I know it was commented out in a recent commit that I approved personally. Maybe look at a warped fungus without any texture packs with your change and you'll see why.

Yeah, I commented it out, because when I made the new graphic for the fungus, no one mentioned to change the hitbox. If you like, feel free to remove the hitbox code completely. I think that would honestly be best, as it's going to be drawn differently for the different texture packs, and we'll get bug reports if the hitbox is off.

> @SmokeyDope I think the hitbox size was commented out for a reason. I know it was commented out in a recent commit that I approved personally. Maybe look at a warped fungus without any texture packs with your change and you'll see why. Yeah, I commented it out, because when I made the new graphic for the fungus, no one mentioned to change the hitbox. If you like, feel free to remove the hitbox code completely. I think that would honestly be best, as it's going to be drawn differently for the different texture packs, and we'll get bug reports if the hitbox is off.
👍 1
Michieal commented 2023-01-30 08:47:39 +01:00
Member
Copy Link

Also, thank you for picking this up. I always thought that these things should have sounds! lol

Also, thank you for picking this up. I always thought that these things should have sounds! lol
❤️ 1
Michieal commented 2023-01-30 08:51:49 +01:00
Member
Copy Link

This PR fixes #3287

This PR fixes #3287
😆 1
SmokeyDope commented 2023-01-30 15:53:53 +01:00
Author
Member
Copy Link

Also, thank you for picking this up. I always thought that these things should have sounds! lol

Thank you for the kind encouragement! The sound thing bothered me way more than it should have which kick started this whole PR not to mention breaking warped fungus blocks was a nightmare. Figured instead of making 10 bug reports eating into more knowledgeable contributors precious time I could try poking around with the code to fix it myself and was pleasantly suprised with how well it all went. Im glad that there are easy improvements that can be made to the existing code even a complete novice like me can fix up great to cut teeth on.

f you like, feel free to remove the hitbox code completely. I think that would honestly be best, as it's going to be drawn differently for the different texture packs, and we'll get bug reports if the hitbox is off.

Ill see what I can do with removing the fungus hitbox great suggestion

> Also, thank you for picking this up. I always thought that these things should have sounds! lol Thank you for the kind encouragement! The sound thing bothered me way more than it should have which kick started this whole PR not to mention breaking warped fungus blocks was a nightmare. Figured instead of making 10 bug reports eating into more knowledgeable contributors precious time I could *try* poking around with the code to fix it myself and was pleasantly suprised with how well it all went. Im glad that there are easy improvements that can be made to the existing code even a complete novice like me can fix up great to cut teeth on. >f you like, feel free to remove the hitbox code completely. I think that would honestly be best, as it's going to be drawn differently for the different texture packs, and we'll get bug reports if the hitbox is off. Ill see what I can do with removing the fungus hitbox great suggestion
❤️ 2
SmokeyDope added 1 commit 2023-01-30 18:09:57 +01:00
32b5b435c5 Make weeping vines and twisting vines break instantly 
Partial revert of previous commit, they should break instantly
Michieal commented 2023-01-30 20:51:10 +01:00
Member
Copy Link

Ill see what I can do with removing the fungus hitbox great suggestion

Thank you.

Thank you for the kind encouragement! The sound thing bothered me way more than it should have which kick started this whole PR not to mention breaking warped fungus blocks was a nightmare. Figured instead of making 10 bug reports eating into more knowledgeable contributors precious time I could try poking around with the code to fix it myself and was pleasantly suprised with how well it all went. Im glad that there are easy improvements that can be made to the existing code even a complete novice like me can fix up great to cut teeth on.

Very welcome! A lot of things in here are good to cut your teeth on. My one piece of advice... when you make a PR, state in the PR what it does, and what "scope" you are changing... and then try to stick to that scope. (Scope is the term for the area or reach that a change is in / covers.)

I offer this, because it's easy to go out of scope and go overboard. Not saying that you will do that, but others have done such before. Also, going out of scope will delay getting fixes / changes merged as it becomes a nightmare to test, as we have to test all changes. (for actual changes that are mixed in with prettifying code, this becomes a nightmare as almost every line of code is flagged as "changed.") So, try to be mindful is what I am saying. :)

> Ill see what I can do with removing the fungus hitbox great suggestion Thank you. > Thank you for the kind encouragement! The sound thing bothered me way more than it should have which kick started this whole PR not to mention breaking warped fungus blocks was a nightmare. Figured instead of making 10 bug reports eating into more knowledgeable contributors precious time I could try poking around with the code to fix it myself and was pleasantly suprised with how well it all went. Im glad that there are easy improvements that can be made to the existing code even a complete novice like me can fix up great to cut teeth on. Very welcome! A lot of things in here are good to cut your teeth on. My one piece of advice... when you make a PR, state in the PR what it does, and what "scope" you are changing... and then try to stick to that scope. (Scope is the term for the area or reach that a change is in / covers.) I offer this, because it's easy to go out of scope and go overboard. Not saying that you will do that, but others have done such before. Also, going out of scope will delay getting fixes / changes merged as it becomes a nightmare to test, as we have to test all changes. (for actual changes that are mixed in with prettifying code, this becomes a nightmare as almost every line of code is flagged as "changed.") So, try to be mindful is what I am saying. :)
👍 1
SmokeyDope commented 2023-02-01 03:42:57 +01:00
Author
Member
Copy Link

Totally agree! I have a feeling that this pr is more or less good to go.

Totally agree! I have a feeling that this pr is more or less good to go.
❤️ 1
SmokeyDope changed title from WIP: fix incorrect digtypes and missing sounds for some nether blocks to fix incorrect digtypes and missing sounds for some nether blocks 2023-02-01 03:43:06 +01:00
ancientmarinerdev requested changes 2023-02-02 17:29:27 +01:00
ancientmarinerdev left a comment
Owner
Copy Link

Hey SmokeyDope. Really appreciate you taking the time to look into this PR. I've left some comments for some improvements. If you need any help or clarification, please reply back or feel free to message me on Discord.

Thanks.

Hey SmokeyDope. Really appreciate you taking the time to look into this PR. I've left some comments for some improvements. If you need any help or clarification, please reply back or feel free to message me on Discord. Thanks.
mods/ITEMS/mcl_crimson/init.lua Outdated
@ -77,3 +77,3 @@
groups = {dig_immediate=3,mushroom=1,attached_node=1,dig_by_water=1,destroy_by_lava_flow=1,dig_by_piston=1,enderman_takable=1,deco_block=1},
light_source = 1,
--[[ selection_box = {
sounds = mcl_sounds.node_sound_leaves_defaults(),
ancientmarinerdev commented 2023-02-02 17:05:51 +01:00
Owner
Copy Link

Please can you keep indentation in line with others. We use tabs, not spaces.

Please can you keep indentation in line with others. We use tabs, not spaces.
👍 1
ancientmarinerdev marked this conversation as resolved
mods/ITEMS/mcl_crimson/init.lua Outdated
@ -113,3 +114,3 @@
climbable = true,
buildable_to = true,
groups = {dig_immediate=3,vines=1,dig_by_water=1,destroy_by_lava_flow=1,dig_by_piston=1,deco_block=1, shearsy = 1},
groups = {dig_immediate=3, handy=1, axey=1, shearsy=1, swordy=1, vines=1,
ancientmarinerdev commented 2023-02-02 17:08:29 +01:00
Owner
Copy Link

Should this really be swordy, axey, handy? That's more changes than I expected.

Should this really be swordy, axey, handy? That's more changes than I expected.
👍 1
SmokeyDope commented 2023-02-02 22:32:22 +01:00
Author
Member
Copy Link

handy, axey, and swordy were copied over from regular vines groups when I incorrectly assumed that nether vines shares most atributes with overworld ones.

Just removed handy, axey, and swordy from the nether vines and all tools seem to still work as they should (~30% chance of dropping item unless shear which is 100%

Sorry about that!

handy, axey, and swordy were copied over from regular vines groups when I incorrectly assumed that nether vines shares most atributes with overworld ones. Just removed handy, axey, and swordy from the nether vines and all tools seem to still work as they should (~30% chance of dropping item unless shear which is 100% Sorry about that!
Michieal commented 2023-02-03 01:46:52 +01:00
Member
Copy Link

it looks like (from what it shows here) that you took out the destroyed by lava, the piston thing, etc. Are those on a different line?

it looks like (from what it shows here) that you took out the destroyed by lava, the piston thing, etc. Are those on a different line?
SmokeyDope commented 2023-02-03 01:48:44 +01:00
Author
Member
Copy Link

Yeah those were on a different line, theyre all back on one line now

Yeah those were on a different line, theyre all back on one line now
ancientmarinerdev marked this conversation as resolved
mods/ITEMS/mcl_crimson/init.lua Outdated
@ -114,2 +115,3 @@
buildable_to = true,
groups = {dig_immediate=3,vines=1,dig_by_water=1,destroy_by_lava_flow=1,dig_by_piston=1,deco_block=1, shearsy = 1},
groups = {dig_immediate=3, handy=1, axey=1, shearsy=1, swordy=1, vines=1,
dig_by_water=1, destroy_by_lava_flow=1, dig_by_piston=1, deco_block=1},
ancientmarinerdev commented 2023-02-02 17:08:40 +01:00
Owner
Copy Link

Please can you keep indentation in line with others. We use tabs, not spaces.

Please can you keep indentation in line with others. We use tabs, not spaces.
👍 1
ancientmarinerdev marked this conversation as resolved
mods/ITEMS/mcl_crimson/init.lua Outdated
@ -128,7 +131,17 @@ minetest.register_node("mcl_crimson:twisting_vines", {
if not minetest.is_creative_enabled(clicker:get_player_name()) then
itemstack:take_item()
end
--placement checks sucess, put twisting vine
ancientmarinerdev commented 2023-02-02 17:10:12 +01:00
Owner
Copy Link

I think this comment is unnecessary because the next line is clear what is happening.

I think this comment is unnecessary because the next line is clear what is happening.
👍 1
SmokeyDope commented 2023-02-02 22:39:44 +01:00
Author
Member
Copy Link

100% agree

100% agree
ancientmarinerdev marked this conversation as resolved
mods/ITEMS/mcl_crimson/init.lua Outdated
@ -130,2 +133,4 @@
end
--placement checks sucess, put twisting vine
grow_vines(pos, 1, "mcl_crimson:twisting_vines")
--add sound to vine placement
ancientmarinerdev commented 2023-02-02 17:09:35 +01:00
Owner
Copy Link

Please can you keep indentation on this whole block so it is in line with others. We use tabs, not spaces.

Please can you keep indentation on this whole block so it is in line with others. We use tabs, not spaces.
👍 1
ancientmarinerdev marked this conversation as resolved
mods/ITEMS/mcl_crimson/init.lua Outdated
@ -132,0 +136,4 @@
--add sound to vine placement
local idef = itemstack:get_definition()
local itemstack, success = minetest.item_place_node(itemstack, placer, pointed_thing)
if success then
ancientmarinerdev commented 2023-02-02 17:10:37 +01:00
Owner
Copy Link

Please can you keep indentation for this whole block so that it is in line with others. We use tabs, not spaces.

Please can you keep indentation for this whole block so that it is in line with others. We use tabs, not spaces.
👍 1
ancientmarinerdev marked this conversation as resolved
mods/ITEMS/mcl_crimson/init.lua
@ -137,6 +150,16 @@ minetest.register_node("mcl_crimson:twisting_vines", {
end
return itemstack
end,
--breaking twisting vines breaks the vines above logic
ancientmarinerdev commented 2023-02-02 17:15:41 +01:00
Owner
Copy Link

I think this comment is unneccessary as it's in the on_dig and the variables are well named (abovenode).

I think this comment is unneccessary as it's in the on_dig and the variables are well named (abovenode).
👍 1
ancientmarinerdev marked this conversation as resolved
mods/ITEMS/mcl_crimson/init.lua Outdated
@ -139,1 +152,4 @@
end,
--breaking twisting vines breaks the vines above logic
on_dig = function(pos, node, digger)
local above = {x=pos.x, y=pos.y+1, z=pos.z}
ancientmarinerdev commented 2023-02-02 17:18:29 +01:00
Owner
Copy Link

Ideally we need to use the new way of creating vectors as this way is deprecated.

vector.new(x,y,z)

More correctly, in this case, it should be:

vector.offset(pos,0,+1,0)

Ideally we need to use the new way of creating vectors as this way is deprecated. vector.new(x,y,z) More correctly, in this case, it should be: vector.offset(pos,0,+1,0)
👍 1
Michieal commented 2023-02-03 01:59:57 +01:00
Member
Copy Link

vector.offset(pos, 0, 1, 0) also works well.

`vector.offset(pos, 0, 1, 0)` also works well.
ancientmarinerdev commented 2023-02-04 01:31:46 +01:00
Owner
Copy Link

Oh gosh, I realised I gave you incorrect code on that! Ha. Sorry about that.

Good catch Michieal.

Oh gosh, I realised I gave you incorrect code on that! Ha. Sorry about that. Good catch Michieal.
mods/ITEMS/mcl_crimson/init.lua Outdated
@ -169,3 +193,3 @@
climbable = true,
buildable_to = true,
groups = {dig_immediate=3,vines=1,dig_by_water=1,destroy_by_lava_flow=1,dig_by_piston=1,deco_block=1, shearsy = 1},
groups = {dig_immediate=3, handy=1, axey=1, shearsy=1, swordy=1, vines=1, dig_by_water=1,
ancientmarinerdev commented 2023-02-02 17:19:22 +01:00
Owner
Copy Link

That's a lot of new groups, are these all correct? Swordy, axey and handy?

That's a lot of new groups, are these all correct? Swordy, axey and handy?
👍 1
ancientmarinerdev marked this conversation as resolved
mods/ITEMS/mcl_crimson/init.lua Outdated
@ -187,1 +213,4 @@
--placement check sucess, grow weeping vine
grow_vines(pos, 1, "mcl_crimson:weeping_vines", -1)
--add sound to placement
local idef = itemstack:get_definition()
ancientmarinerdev commented 2023-02-02 17:19:44 +01:00
Owner
Copy Link

Please resolve indentation for this block, please.

Please resolve indentation for this block, please.
👍 1
ancientmarinerdev marked this conversation as resolved
mods/ITEMS/mcl_crimson/init.lua Outdated
@ -195,1 +231,4 @@
end,
--breaking weeping vines breaks the vines below logic
on_dig = function(pos, node, digger)
local below = {x=pos.x, y=pos.y-1, z=pos.z}
ancientmarinerdev commented 2023-02-02 17:20:22 +01:00
Owner
Copy Link

please use vector.offset

please use vector.offset
👍 1
ancientmarinerdev marked this conversation as resolved
mods/ITEMS/mcl_crimson/init.lua Outdated
@ -196,0 +239,4 @@
end
end,
ancientmarinerdev commented 2023-02-02 17:22:31 +01:00
Owner
Copy Link

I don't think we need these extra lines.

I don't think we need these extra lines.
👍 1
ancientmarinerdev marked this conversation as resolved
mods/ITEMS/mcl_crimson/init.lua Outdated
@ -268,1 +317,3 @@
_mcl_hardness = 2,
groups = {handy = 1, hoey = 7, swordy = 1, deco_block = 1},
sounds = mcl_sounds.node_sound_leaves_defaults(
{
ancientmarinerdev commented 2023-02-02 17:24:32 +01:00
Owner
Copy Link

Could we resolve indentation for this. I think it should be ({

and the last }), should be inline with the _mcl_hardness

Could we resolve indentation for this. I think it should be ({ and the last }), should be inline with the _mcl_hardness
👍 1
ancientmarinerdev marked this conversation as resolved
mods/ITEMS/mcl_crimson/init.lua Outdated
@ -275,2 +330,3 @@
groups = {handy = 1, hoey = 7, swordy = 1, deco_block = 1},
light_source = minetest.LIGHT_MAX,
_mcl_hardness = 2,
sounds = mcl_sounds.node_sound_leaves_defaults(
ancientmarinerdev commented 2023-02-02 17:25:00 +01:00
Owner
Copy Link

Please can you resolve bracket indentation, please?

Please can you resolve bracket indentation, please?
👍 1
ancientmarinerdev marked this conversation as resolved
mods/ITEMS/mcl_nether/init.lua Outdated
@ -389,12 +389,12 @@ minetest.register_craft({
})
minetest.register_craft({
type = "shapeless",
ancientmarinerdev commented 2023-02-02 17:26:03 +01:00
Owner
Copy Link

Not sure why this is double indented.

Not sure why this is double indented.
👍 1
ancientmarinerdev marked this conversation as resolved
SmokeyDope added 1 commit 2023-02-02 22:37:32 +01:00
SmokeyDope added 1 commit 2023-02-02 23:08:52 +01:00
SmokeyDope added 1 commit 2023-02-02 23:30:34 +01:00
SmokeyDope added 1 commit 2023-02-02 23:36:49 +01:00
SmokeyDope added 1 commit 2023-02-02 23:39:52 +01:00
SmokeyDope added 1 commit 2023-02-02 23:42:58 +01:00
SmokeyDope added 1 commit 2023-02-02 23:45:23 +01:00
SmokeyDope added 1 commit 2023-02-03 00:12:25 +01:00
SmokeyDope added 1 commit 2023-02-03 01:43:24 +01:00
SmokeyDope added 1 commit 2023-02-03 01:50:32 +01:00
SmokeyDope requested review from ancientmarinerdev 2023-02-03 01:53:56 +01:00
Michieal commented 2023-02-03 06:33:28 +01:00
Member
Copy Link

Does this fix #2704 ?

Does this fix #2704 ?
ancientmarinerdev approved these changes 2023-02-04 01:25:19 +01:00
ancientmarinerdev left a comment
Owner
Copy Link

Looks really good. Thanks for taking the feedback on board and implementing it. You've solved quite a few problems here. It is really appreciated.

For next time, I would recommend:

  1. creating a branch off master (creating a PR from master to ours could be a recipe for disaster, but it looks ok this time)
  2. breaking it down in to multiple PRs (It's easier to review and test if it's smaller. The block digging could have been kept separate from the planty stuff and it might get picked up sooner, and can be easier to finish which helps with the motivation)
  3. you don't need that many commits (you can resolve all the indentation issues in 1 commit). not an issue, but it can pollute version history a bit when people are later working on files and trying to understand the code and changes)
  4. I did leave a minor indentation issue comment, but it's a massive improvement so I'm happy to let that slide. It's worth keeping an eye on these, it makes code easier to maintain as you can see what code is in a block or function at a glance) and most IDE's do this automatically (and in the correct format, tabs if you configure it)

Thanks for picking this up!

Welcome aboard. :)

Looks really good. Thanks for taking the feedback on board and implementing it. You've solved quite a few problems here. It is really appreciated. For next time, I would recommend: 1. creating a branch off master (creating a PR from master to ours could be a recipe for disaster, but it looks ok this time) 2. breaking it down in to multiple PRs (It's easier to review and test if it's smaller. The block digging could have been kept separate from the planty stuff and it might get picked up sooner, and can be easier to finish which helps with the motivation) 3. you don't need that many commits (you can resolve all the indentation issues in 1 commit). not an issue, but it can pollute version history a bit when people are later working on files and trying to understand the code and changes) 4. I did leave a minor indentation issue comment, but it's a massive improvement so I'm happy to let that slide. It's worth keeping an eye on these, it makes code easier to maintain as you can see what code is in a block or function at a glance) and most IDE's do this automatically (and in the correct format, tabs if you configure it) Thanks for picking this up! Welcome aboard. :)
❤️ 1
mods/ITEMS/mcl_crimson/init.lua
@ -269,0 +304,4 @@
groups = {handy = 1, hoey = 7, swordy = 1, deco_block = 1},
_mcl_hardness = 1,
sounds = mcl_sounds.node_sound_leaves_defaults({
footstep={name="default_dirt_footstep", gain=0.7},
ancientmarinerdev commented 2023-02-04 01:13:30 +01:00
Owner
Copy Link

These lines are double indented. Not a problem but a note for next time.

These lines are double indented. Not a problem but a note for next time.
mods/ITEMS/mcl_crimson/init.lua
@ -276,1 +317,3 @@
_mcl_hardness = 2,
_mcl_hardness = 1,
sounds = mcl_sounds.node_sound_leaves_defaults({
footstep={name="default_dirt_footstep", gain=0.7},
ancientmarinerdev commented 2023-02-04 01:14:27 +01:00
Owner
Copy Link

Doubley indented. Only needs to be tab indented once. Not a problem, but a note for next time.

Doubley indented. Only needs to be tab indented once. Not a problem, but a note for next time.
ancientmarinerdev merged commit 58a08ea697 into master 2023-02-04 01:27:50 +01:00
ancientmarinerdev added this to the 0.83.0 - Safe and Sound milestone 2023-02-04 01:28:29 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
ancientmarinerdev
Labels
Clear labels   
#P1 CRITICAL

for crashes, catastrophic bugs, etc.

  
#P2: HIGH

highest priority level

  
#P3: elevated

2nd-highest priority level

  
#P4 priority: medium

for things that aren't elevated but have been prioritised

  
#P6: low

low priority level

  
#Review

Tickets that may improve or fix an issue but will need a review after a short period of time

  
annoying

non-bugs that annoys people

  
API

   
bug

   
code quality

   
combat

   
commands

   
compatibility

Label to cover migration issues from old worlds or mcl5

  
configurability

   
contribution inside

   
controls

   
core feature

   
creative mode

   
delayed for engine release

   
documentation

   
duplicate

   
enhancement

enhancement request/suggestion

  
environment

Issues that affect weather or light

  
feature request

   
gameplay

   
graphics

   
ground content conflict

mapgen gc-specific quirks

  
GUI/HUD

   
help wanted

   
incomplete feature

   
invalid / won't fix

Invalid, or will not fix

  
items

   
looking for contributor

   
mapgen

   
meta

Relating to VoxeLibre on Mesehub and elsewhere

  
mineclone2+

fit for MineClone2-Plus

  
Minecraft >= 1.13

   
Minecraft >= 1.17

   
missing feature

   
mobile

involving Minetest on mobile devices (android)

  
mobs

   
mod support

Issues related to mod support

  
model needed

A model is needed to be created or updated

  
multiplayer

Issues affecting multiplayer

  
Needs adoption

PR needs author action or to be adopted so changes can be implemented to get the PR closer to merge

  
needs discussion

   
needs engine change

   
needs more information

   
needs research

   
nodes

   
non-mob entities

   
performance

   
player

issues related to the player

  
possible close

   
redstone

   
release notes

This ticket will need information adding to release notes

  
schematics

   
Skyblock

   
sounds

   
Testing / Retest

This needs testing or retesting

  
tools

Related to the tools scripts, which aren't run in game

  
translation

   
unconfirmed
  
mcl5

Possibly relevant for MineClone5

  
mcla

Possibly relevant for Mineclonia

  
Media missing

media files for this feature are missing - contributions welcome

No Label
#P1 CRITICAL
#P2: HIGH
#P3: elevated
#P4 priority: medium
#P6: low
#Review
annoying
API
bug
code quality
combat
commands
compatibility
configurability
contribution inside
controls
core feature
creative mode
delayed for engine release
documentation
duplicate
enhancement
environment
feature request
gameplay
graphics
ground content conflict
GUI/HUD
help wanted
incomplete feature
invalid / won't fix
items
looking for contributor
mapgen
meta
mineclone2+
Minecraft >= 1.13
Minecraft >= 1.17
missing feature
mobile
mobs
mod support
model needed
multiplayer
Needs adoption
needs discussion
needs engine change
needs more information
needs research
nodes
non-mob entities
performance
player
possible close
redstone
release notes
schematics
Skyblock
sounds
Testing / Retest
tools
translation
unconfirmed
mcl5
mcla
Media missing
Milestone
Clear milestone
No items
No Milestone
0.83.0 - Safe and Sound
Projects
Clear projects
No project
Assignees
Clear assignees
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#3351
No description provided.
Delete Branch "SmokeyDope/MineClone2:master"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?