getDigParams: Fix selecting the lowest digging time #5558

HybridDog · 2017-04-10T18:06:42Z

Tidy up code
Fix selecting the best fitting time: time was compared with result_time before dividing it by the level difference
Remove dead code (time from last punch)

SmallJoker · 2017-04-10T18:17:33Z

src/tool.cpp

 	}

-	u16 wear_i = 65535.*result_wear;
+	u16 wear_i = 0xffff * result_wear;


SmallJoker

Otherwise 👍

nerzhul · 2017-04-10T19:19:19Z

src/tool.cpp

-					result_wear = 0;
-				result_main_group = name;
-			}
+		bool time_exists = cap.getTime(itemgroup_get(groups, groupname), &time);


no need to create variable, just call this into condition

It's not entirely clear what getTime returns, thus it's better to keep it this way. Makes it easier to understand and the compiler would optimize this anyway.

SmallJoker

LGTM, however, you quite misunderstood what nerzhul and me actually meant. Your previous commit was good too.

SmallJoker · 2017-04-11T14:28:05Z

src/tool.cpp

-		//infostream<<"f="<<f<<std::endl;
-		result_time /= f;
-		result_wear /= f;
+		if (f > 1.0) {


This is wrong. It can't be > 1 since last_punch < punch_interval

You're right. Anyway, l just noticed ($ ag -r -A2 getDigParams) that it's unused code.
l'll remove it in a separate commit.

Found issue.

SmallJoker · 2017-04-18T18:13:31Z

src/script/lua_api/l_util.cpp

@@ -219,18 +219,14 @@ int ModApiUtil::l_write_json(lua_State *L)
 	return 1;
 }

-// get_dig_params(groups, tool_capabilities[, time_from_last_punch])
+// get_dig_params(groups, tool_capabilities)
 int ModApiUtil::l_get_dig_params(lua_State *L)


Mind documenting get_hit_params and get_dig_params in the Lua API or does that rather belong into a new pull/issue?

lt belongs to another PR.

paramat · 2017-04-19T17:25:49Z

l put the first two in one commit because the code is changed in the same place.

Code cleanup should still be a separate commit even if it is acting on the same code, mixiing cleanups with other changes makes review harder and creates a more confusing history.

HybridDog · 2017-04-19T19:05:41Z

paramat, you're right. l added an extra commit which shows a simple (and inefficient) fix.

paramat · 2017-04-21T19:09:41Z

Thanks, perhaps the last 2 can be squashed? (this could be done on merge).

sfan5 · 2017-04-29T22:35:40Z

Only thing that failed is an unimportant unittest on OS X

time was compared with result_time before dividing it by the level difference

sfan5

lgtm

paramat · 2017-12-26T23:04:30Z

@SmallJoker still +1? If so this can be merged.

SmallJoker · 2017-12-28T16:29:10Z

@paramat Yes.

paramat · 2018-01-03T04:11:10Z

d7c1f6c
345e104

paramat · 2018-01-03T04:16:01Z

Is there a corresponding issue to close?

HybridDog · 2018-01-03T09:09:28Z

AfaIk, there isn't.

SmallJoker reviewed Apr 10, 2017

View reviewed changes

HybridDog force-pushed the toolfix branch from 2c9df66 to 7df342e Compare April 10, 2017 19:14

nerzhul reviewed Apr 10, 2017

View reviewed changes

HybridDog force-pushed the toolfix branch from 7df342e to 9fe00c0 Compare April 10, 2017 19:28

paramat added the Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements label Apr 10, 2017

SmallJoker previously approved these changes Apr 11, 2017

View reviewed changes

SmallJoker reviewed Apr 11, 2017

View reviewed changes

HybridDog force-pushed the toolfix branch from 9fe00c0 to 4c6476a Compare April 11, 2017 15:02

HybridDog changed the title ~~tool.cpp: 3 changes in getDigParams~~ 3 changes in getDigParams Apr 11, 2017

SmallJoker reviewed Apr 18, 2017

View reviewed changes

nerzhul added the Rebase needed The PR needs to be rebased by its author label Apr 19, 2017

HybridDog force-pushed the toolfix branch 2 times, most recently from 3409f33 to 8874520 Compare April 19, 2017 19:01

HybridDog changed the title ~~3 changes in getDigParams~~ getDigParams: Fix selecting the lowest digging time Apr 19, 2017

sfan5 removed the Rebase needed The PR needs to be rebased by its author label Apr 20, 2017

Fixer-007 mentioned this pull request Apr 21, 2017

Torches are not instamined anymore minetest/minetest_game#1706

Closed

HybridDog force-pushed the toolfix branch from 8874520 to 4643f5f Compare April 28, 2017 18:36

SmallJoker added the Rebase needed The PR needs to be rebased by its author label Aug 29, 2017

HybridDog added 3 commits October 14, 2017 16:18

Fix selecting the best fitting time

0efb707

time was compared with result_time before dividing it by the level difference

Tidy up code

b9855b8

Remove dead code

a9d1f5b

HybridDog force-pushed the toolfix branch from 4643f5f to a9d1f5b Compare October 14, 2017 14:26

sfan5 removed the Rebase needed The PR needs to be rebased by its author label Oct 14, 2017

sfan5 approved these changes Oct 14, 2017

View reviewed changes

sfan5 added the One approval ✅ ◻️ label Oct 14, 2017

paramat added >= Two approvals ✅ ✅ and removed One approval ✅ ◻️ labels Dec 26, 2017

paramat closed this Jan 3, 2018

HybridDog deleted the toolfix branch January 3, 2018 09:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

getDigParams: Fix selecting the lowest digging time #5558

getDigParams: Fix selecting the lowest digging time #5558

HybridDog commented Apr 10, 2017 •

edited

Loading

SmallJoker Apr 10, 2017

SmallJoker left a comment

nerzhul Apr 10, 2017

SmallJoker Apr 10, 2017

HybridDog Apr 10, 2017

SmallJoker left a comment •

edited

Loading

SmallJoker Apr 11, 2017

HybridDog Apr 11, 2017

SmallJoker Apr 18, 2017

HybridDog Apr 19, 2017

paramat commented Apr 19, 2017 •

edited

Loading

HybridDog commented Apr 19, 2017 •

edited

Loading

paramat commented Apr 21, 2017

sfan5 commented Apr 29, 2017

sfan5 left a comment

paramat commented Dec 26, 2017

SmallJoker commented Dec 28, 2017

paramat commented Jan 3, 2018

paramat commented Jan 3, 2018

HybridDog commented Jan 3, 2018

getDigParams: Fix selecting the lowest digging time #5558

getDigParams: Fix selecting the lowest digging time #5558

Conversation

HybridDog commented Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

SmallJoker left a 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

SmallJoker left a comment • 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

paramat commented Apr 19, 2017 • edited Loading

HybridDog commented Apr 19, 2017 • edited Loading

paramat commented Apr 21, 2017

sfan5 commented Apr 29, 2017

sfan5 left a comment

Choose a reason for hiding this comment

paramat commented Dec 26, 2017

SmallJoker commented Dec 28, 2017

paramat commented Jan 3, 2018

paramat commented Jan 3, 2018

HybridDog commented Jan 3, 2018

HybridDog commented Apr 10, 2017 •

edited

Loading

SmallJoker left a comment •

edited

Loading

paramat commented Apr 19, 2017 •

edited

Loading

HybridDog commented Apr 19, 2017 •

edited

Loading