New MV Hydro Machine #412

tanmayameher · 2018-02-06T06:32:50Z

Decreased the value of LV hydro to 175 EU (quite easy to craft)

Upgrade on LV hydro to MV hydro (require a little resource and gives 10x more power)

Textures uploaded too.

Please refer to This discussion HERE
Please comment and give a review

Thanks and regards,

The LV hydro is easy to make giving lot of power. The New hydro MV will put a tier system to it; thereby giving more incentive to player to pursue MV hydro plus a little survival aspect. This is a result of [Detailed discussion which is here](minetest-mods#411). Thanks to VanessaE for a good talk and support :)

The LV hydro is easy to make giving lot of power. The New hydro MV will put a tier system to it; thereby giving more incentive to player to pursue MV hydro plus a little survival aspect. This is a result of [Detailed discussion which is here](minetest-mods#411). Thanks to VanessaE for a good talk and support and enthusiasm to make one :) This will now produce around 175 EU (in between 150-200, so basically average). The MV hydro will give 10x more power than this one. :)

The LV hydro is easy to make giving lot of power. The New hydro MV will put a tier system to it; thereby giving more incentive to player to pursue MV hydro plus a little survival aspect. This is a result of [Detailed discussion which is here](minetest-mods#411). Thanks to VanessaE for a good talk and support :)

numberZero

I suppose you copypasted original water mill code. That’s not a good practice, you should use the same code when possible; see solar arrays for an example.

numberZero · 2018-02-07T22:09:54Z

technic/machines/MV/hydro_turbine.lua

+local run = function(pos, node)
+	local meta             = minetest.get_meta(pos)
+	local water_flow       = 0
+	local lava_nodes       = 0


I have no idea why lava node is zero there. May be it is there to check some exception or something. Again result of copy pasting entire code from another file xD. btw, should i remove that local lava_nodes?! Don't know whether it will start doing some runtime crazy things. Happens to me all the time, when i try to change a line in minetest mods and it entirely crash on me ;)

It is there because the water mill code itself is apparently copied from the geothermal generator =D

numberZero · 2018-02-07T22:11:24Z

technic/machines/MV/hydro_turbine.lua

+	local lava_nodes       = 0
+	local production_level = 0
+	local eu_supply        = 0
+	local max_output       = 35 * 50 -- 10 times better than LV hydro because of 2 diamonds extra and 4 stainless steel, a transformer and whatnot ;P. If 2 extra diamonds feels too hard to get; change it to mese may be. But i don't think that's a good idea. I would rather go for mese block or something if changing. Man hydro turbines are strong and long lasting. So, give it some value :)


Too long comment. You’d better write that in PR description rather than in source.

ok. is there any function comment / more precisely documenting a method (as in python). Sorry, i have almost negligible experience of lua. It will be helpful if i will put in there :)

There is LuaDoc. But still, comments are there to describe API (e.g. what does that function do and what arguments does it expect)

numberZero · 2018-02-07T22:12:20Z

technic/machines/MV/hydro_turbine.lua

+	recipe = {
+		{'technic:stainless_steel_ingot', 'technic:water_mill',        'technic:stainless_steel_ingot'},
+		{'default:diamond',     'technic:mv_transformer', 'default:diamond'},
+		{'technic:stainless_steel_ingot', 'technic:mv_cable',       'technic:stainless_steel_ingot'},


Misaligned. Either remove alignment (not to be confused with indent) or do it properly.

Sorry, again that was a copy paste or may be due to long names of stainless_steel ingots. i will align it. Should have used a lua editor or a plugin on my editor 👍

numberZero · 2018-02-07T22:16:00Z

technic/machines/LV/water_mill.lua

@@ -32,8 +32,7 @@ local run = function(pos, node)
 	local lava_nodes       = 0
 	local production_level = 0
 	local eu_supply        = 0
-	local max_output       = 35 * 45 -- four param2's at 15 makes 60, cap it lower for "overload protection"
-									 -- (plus we want the gen to report 100% if three sides have full flow)
+	local max_output       = 35 * 5 -- keeping it around 175 mid value little more than previous 150 :)


45 here was maximum flow, while 35 was generation per “flow unit,” i.e. what is to be changed. You can change both, though, changing both maximum flow and generation per flow unit. But don’t forget to change eu_supply formula accordingly.

yeah, i pretty much copy pasted, with not even a variable name change. So, i guess, that was the problem. sorry about that. :) Ok, may be i will change the generation per flow unit. I mean why change a constant or a max value unnecessarily. Thanks for clarification 👍

numberZero · 2018-02-07T22:26:42Z

technic/machines/MV/hydro_turbine.lua

+	recipe = {
+		{'technic:stainless_steel_ingot', 'technic:water_mill',        'technic:stainless_steel_ingot'},
+		{'default:diamond',     'technic:mv_transformer', 'default:diamond'},
+		{'technic:stainless_steel_ingot', 'technic:mv_cable',       'technic:stainless_steel_ingot'},


You could use water mills instead of diamonds in the recipe, similar to solar arrays crafting.

Yes, that's a good idea, i guess. Will be little bit tougher 3 water mills instead of 2 diamonds and 1 water mill right 👍

numberZero · 2018-02-07T22:29:44Z

technic/machines/MV/hydro_turbine.lua

+	  or node.name == "default:river_water_flowing" then
+		return node.param2 -- returns approx. water flow, if any
+	end
+	return false


return 0 to simplify usage

But, i think lua takes '0' as true ; so in this case, i should do return 1

Er? Yes it does treat any number as true, but if you return 0 you don’t need to check for that at all as it’s perfectly safe to add zero to any number)

numberZero · 2018-02-07T22:30:45Z

technic/machines/MV/hydro_turbine.lua

+	}
+})
+
+local function check_node_around_mill(pos)


get_water_flow seems to be a better name

I have no idea of this function. As you said copy paste :P. May be i will change the name. It's the same name in LV hydro too.

numberZero · 2018-02-07T22:32:12Z

technic/machines/MV/hydro_turbine.lua

+
+	if production_level > 0 and
+	   minetest.get_node(pos).name == "technic:hydro_turbine" then
+		technic.swap_node (pos, "technic:hydro_turbine_active")


extra space

space, not blank line:
technic.swap_node (pos,
should be:
technic.swap_node(pos,
as in all surrounding code.
See http://dev.minetest.net/Lua_code_style_guidelines for details (but please don’t use double blank lines unless asked explicitly. That rule came from Python where functions are ended implicitly and not with a keyword as in Lua).

Thanks to [numberZero](https://github.com/numberZero) for a lot of clarification. Changed the generation per flow unit rather than the maximum flow.

@numberZero

According to the discussion and clarification by @numberZero; Some errors and update has been done. Please refer to [HERE](minetest-mods#412)

Some edits and corrections. Refer to the discussion and suggestions [HERE](minetest-mods#412)

Since Lead and Sulfur are missing from manual and they play an important role in making RE battery i thought it will be important to add them to Manual. Plus, Lead blocks got a place now in HV nuclear reactor layout.

lava node initialization not needed with water mill

Lava node initialization is not needed in water mill

updation of 2 branches unnecessarily created

merging my own branches

numberZero

LGTM, except of:

eu_supply = math.min(35 * water_flow, max_output)
in water mill. It should probably be
eu_supply = math.min(4 * water_flow, max_output)
as otherwise even little water flow make it produce max output possible. New MV Hydro Machine #412 (comment)
This PR is apparently mixed with Update of substances > ore with lead and sulfur #414, despite the fact you used separate branches for them.

And thank you for the PR, it is great for balancing technic power generation)

numberZero · 2018-02-15T21:55:45Z

technic/machines/MV/hydro_turbine.lua

+
+	for _, p in pairs(positions) do
+		local check = get_water_flow(p)
+		if check then


check is always true. #412 (comment)

oops..., forgot do the change in eu_supply formula accordingly in LV water_mill.lua file. Thanks for spotting 💯

Yes, apparently due to some confusion i mixed those 2 branches which has resulted in this. I will revert the changes asap. Thanks 👍

numberZero · 2018-02-15T21:57:33Z

technic/machines/MV/init.lua

@@ -13,7 +13,7 @@ if technic.config:get_bool("enable_wind_mill") then
 end
 dofile(path.."/generator.lua")
 dofile(path.."/solar_array.lua")
-
+dofile(path.."/hydro_turbine.lua")
 -- Machines


Why removing the blank line before the header?

oh lol no... i actually wrote below the blank line and then it seemed to me as a unnecessary blank line above the dofile() function and rest is there 😱 I will correct that.

numberZero · 2018-02-15T22:16:26Z

updation of 2 branches unnecessarily created

Separate branches are mandatory to avoid mixing of different PRs. That’s the reason of not doing PRs from master. Well, you have to split them now... I hope reverting or dropping merge commits will be enough...

Revert "merging my own branches"

Changing the eu_supply formula accordingly. Refer to [PR review](minetest-mods#412 (review))

Correction in formatting. Refer to [PR review](minetest-mods#412 (review))

tanmayameher · 2018-02-16T07:57:10Z

Thanks @numberZero for a thorough review 🥇

Looking at this PR Files comparison of MV Hydro; things seems to be ok now.

But, i think i got the Manual Update PR still messed up. I am thinking of entirely closing this PR, deleting that branch, creating new one and doing a new PR.
Or,
i can just copy paste the relevant code files from minetest-technic:master i changed in this and update respective files in my branch accordingly.

Will the second method work?! I think it should.
Update: Tried the second method. I think it's ok now. But surely, looking at number of commits one can easily get a very good idea about my stupidity; esp. when number of commits is 22 for 2 paragraph text writing in just one file. Lol, 😁 😆

reverting changes that shouldn't be there. Because, this is a manual update PR :) Refer to minetest-mods#412 (comment) and minetest-mods#412 (comment)

Remove redundant check. Lol, forgot this one. Yes. Thanks 👍

SmallJoker

Also run pngcrush/optipng on the new textures to ensure that they're optimized.

SmallJoker · 2018-07-21T15:26:59Z

technic/machines/MV/hydro_turbine.lua

+local function get_water_flow(pos)
+	local node = minetest.get_node(pos)
+	if node.name == "default:water_flowing"
+	  or node.name == "default:river_water_flowing" then


How about river water?

Wrong indent. Please use two tabs here.

@SmallJoker Thank you for reviewing this code. Sorry, that i am not playing or following this game since 4-5 months now and probably won't come back for an indefinite period of time; it would be nice to know on which lines or line numbers i have to change the indent or is it for the entire file like 2 tabs extra?!

River water. yes. it would be good. But, i am clueless about what change i need?! Truthfully, i have forgotten almost what i did last time in this code section. sorry for that. I am willing to update anything required in these code files but i need some time and guidance. I mean do i have to include river water?! I think it's included in this get_water_flow function. Anywhere else do i need the change too?!

Regards,

It's indeed been a long time since this PR got some attention.
Regarding the indents: it's only for the highlighted line. Spaces should not be used (in this project) to indent the code line.

To make river water work, you can simply check whether the node has the group water with a value of 3.
-> if minetest.get_item_group(node.name, "water") == 3 then (replaces the entire if statement and makes the indent change request superfluous)

I think i have changed it now. @SmallJoker Please take a look.

59e7ba7

Also, new texture files updated after crushing them on pngcrush.

c2478fe

Thanks again for helping 👍
Regards

SmallJoker · 2018-07-21T15:27:34Z

technic/machines/MV/hydro_turbine.lua

+	meta:set_string("infotext",
+		S("Hydro %s Generator"):format("MV").." ("..production_level.."%)")
+	if production_level > 0 and
+	   minetest.get_node(pos).name == "technic:hydro_turbine" then


Wrong indent

Changes for proper indentation and river water update.

Texture updated after crushing it with pngcrush.

Changed some more indentation previously done and reviewed. Got it messed up in the new update while indenting. So, reverting them as they were; only not with spaces but tabs. Removed spaces in the tiles section too.

SmallJoker

Works, looks good.

VanessaE · 2018-08-25T07:20:32Z

I'm okay with the idea I guess, but a few users on my servers will be upset at the reduction in power output of their builds.

tanmayameher · 2018-08-25T08:48:44Z

@SmallJoker Thanks for the review. 👍

@VanessaE Well, that's quite true sometimes ;) Anything you change gonna upset few esp. the established 😝 You can do one thing though like upgrading or giving them MV ones in return of LV ones and distribute LV ones to new player or something. That will give them same or more power. or just may be upgrade their machines with some command like replace worldedit command (you know the regular stuff). They have to report it to you though or to mods of your server. Put it in announcement section and give them a little time.

tanmayameher · 2018-08-26T10:41:15Z

@SmallJoker Thank you again for the review and merging the update. Hope i can come with some more new ideas 😄

SmallJoker · 2018-08-26T11:21:22Z

No problem. Thanks for your contribution! This project stalled for a while but now I'm trying to at least review the PRs ;)

beyondlimits · 2018-09-15T20:48:42Z

Hah, I found out this today on my local map and thought it's a bug. 😄

tanmayameher added 3 commits February 5, 2018 23:15

numberZero suggested changes Feb 7, 2018

View reviewed changes

tanmayameher added 10 commits February 8, 2018 12:49

Changed the generation per flow unit not max flow

5855d9f

Thanks to [numberZero](https://github.com/numberZero) for a lot of clarification. Changed the generation per flow unit rather than the maximum flow.

Some Error correction and Update

782410d

According to the discussion and clarification by @numberZero; Some errors and update has been done. Please refer to [HERE](minetest-mods#412)

Update hydro_turbine.lua

431b05f

Some edits and corrections. Refer to the discussion and suggestions [HERE](minetest-mods#412)

Updated Ores section with Lead and Sulfur

c70ecc0

Since Lead and Sulfur are missing from manual and they play an important role in making RE battery i thought it will be important to add them to Manual. Plus, Lead blocks got a place now in HV nuclear reactor layout.

Updated ores section with lead and sulfur

ce626a5

Since Lead and Sulfur are missing from manual and they play an important role in making RE battery i thought it will be important to add them to Manual. Plus, Lead blocks got a place now in HV nuclear reactor layout.

Updated ores section with Lead and Sulfur

4d15781

Since Lead and Sulfur are missing from manual and they play an important role in making RE battery i thought it will be important to add them to Manual. Plus, Lead blocks got a place now in HV nuclear reactor layout.

Update water_mill.lua

febc5eb

lava node initialization not needed with water mill

Update water_mill.lua

7ec3ca2

Lava node initialization is not needed in water mill

Merge pull request #1 from tanmayameher/MT-technic-dedicated

c2d0d60

updation of 2 branches unnecessarily created

Merge pull request #2 from tanmayameher/MT-technic-manual

a27a9b1

merging my own branches

numberZero reviewed Feb 15, 2018

View reviewed changes

tanmayameher added 4 commits February 16, 2018 12:44

Revert "merging my own branches"

d979143

Merge pull request #4 from tanmayameher/revert-2-MT-technic-manual

db28ab8

Revert "merging my own branches"

Update water_mill.lua

ae430d5

Changing the eu_supply formula accordingly. Refer to [PR review](minetest-mods#412 (review))

Update init.lua

c835ebe

Correction in formatting. Refer to [PR review](minetest-mods#412 (review))

tanmayameher added a commit to tanmayameher/technic that referenced this pull request Feb 16, 2018

Update water_mill.lua

ca060ac

reverting changes that shouldn't be there. Because, this is a manual update PR :) Refer to minetest-mods#412 (comment) and minetest-mods#412 (comment)

tanmayameher added a commit to tanmayameher/technic that referenced this pull request Feb 16, 2018

Delete hydro_turbine.lua

bd89f21

reverting changes that shouldn't be there. Because, this is a manual update PR :) Refer to minetest-mods#412 (comment) and minetest-mods#412 (comment)

tanmayameher added a commit to tanmayameher/technic that referenced this pull request Feb 16, 2018

Update init.lua

043ce5e

reverting changes that shouldn't be there. Because, this is a manual update PR :) Refer to minetest-mods#412 (comment) and minetest-mods#412 (comment)

tanmayameher added a commit to tanmayameher/technic that referenced this pull request Feb 16, 2018

Update water_mill.lua

3f9474b

reverting changes that shouldn't be there. Because, this is a manual update PR :) Refer to minetest-mods#412 (comment) and minetest-mods#412 (comment)

numberZero and others added 2 commits February 16, 2018 19:09

Remove redundant check

1020f67

Merge pull request #5 from numberZero/patch-2

d6ab903

Remove redundant check. Lol, forgot this one. Yes. Thanks 👍

numberZero approved these changes Feb 16, 2018

View reviewed changes

SmallJoker reviewed Jul 21, 2018

View reviewed changes

tanmayameher added 3 commits July 22, 2018 20:21

Update hydro_turbine.lua

59e7ba7

Changes for proper indentation and river water update.

Updated new texture for hydroturbine

c2478fe

Texture updated after crushing it with pngcrush.

Update hydro_turbine.lua

6e5b9c8

Changed some more indentation previously done and reviewed. Got it messed up in the new update while indenting. So, reverting them as they were; only not with spaces but tabs. Removed spaces in the tiles section too.

SmallJoker approved these changes Aug 23, 2018

View reviewed changes

SmallJoker merged commit 2e7859c into minetest-mods:master Aug 25, 2018

New MV Hydro Machine #412

New MV Hydro Machine #412

Conversation

tanmayameher commented Feb 6, 2018

numberZero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanmayameher Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanmayameher Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanmayameher Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

numberZero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanmayameher Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

numberZero commented Feb 15, 2018 • edited Loading

tanmayameher commented Feb 16, 2018 • edited Loading

SmallJoker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanmayameher Jul 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanmayameher Jul 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SmallJoker left a comment

Choose a reason for hiding this comment

VanessaE commented Aug 25, 2018

tanmayameher commented Aug 25, 2018 • edited Loading

tanmayameher commented Aug 26, 2018

SmallJoker commented Aug 26, 2018

beyondlimits commented Sep 15, 2018 • edited Loading

tanmayameher Feb 8, 2018 •

edited

Loading

tanmayameher Feb 8, 2018 •

edited

Loading

tanmayameher Feb 8, 2018 •

edited

Loading

tanmayameher Feb 16, 2018 •

edited

Loading

numberZero commented Feb 15, 2018 •

edited

Loading

tanmayameher commented Feb 16, 2018 •

edited

Loading

tanmayameher Jul 22, 2018 •

edited

Loading

tanmayameher Jul 22, 2018 •

edited

Loading

tanmayameher commented Aug 25, 2018 •

edited

Loading

beyondlimits commented Sep 15, 2018 •

edited

Loading