new look is now customizable and minor behaviour fixes #26

0x098 · 2020-08-16T18:12:11Z

+ui colors now customizable with a third tab in the menu named 'Customization'
+seagull ragdoll spawn only once now (sometimes spawned multiple when killed)
+bait hooking adjustments... so they dont get stuck in displacements and nocollide
+bait release timer so it does not reattach instantly
+bait spawn height offset addition to avoid creating black holes of bait
+removed broken cashreg model (it was the dod version i think)
+stove cooking offset higher since some stuff were not cooked at their set positions
+'b' now opens the menu if cvar fishing_mod_b_opens_always is 1 or greater
+could not buy bait if it was equal to the money player has.
+equipping the fishingrod now messages the player of default bind
+entities fishingrod, bobber and hook are no longer possible to "tool" and "property" (context menu)
+added crosshair to see where the player is aiming (also customizable color)
+minor fixes

+ui colors now customizable with a third tab in the menu named 'Customization' +seagull ragdoll spawn only once now (sometimes spawned multiple when killed) +bait hooking adjustments so they dont get stuck in displacements and nocollide +bait release timer so it does not reattach instantly +bait spawn height offset addition to avoid creating black holes of bait +removed broken cashreg model +stove cooking offset higher since some stuff were not cooked at their set positions +'b' now opens the menu if its bound to '+zoom' or not at all +could not buy bait if it was equal to the money player has. +equipping the fishingrod now messages the player of default bind +entities fishingrod, bobber and hook are no longer possible to "tool" and "property" (context menu) +added crosshair to see where the player is aiming (also customizable) +minor fixes

lua/fishing_mod/cl_init.lua

Myaats

Did a quick read through, these are some of the biggest issues in my view. But from now on please never do another mega commit with multiple changes. It makes it almost impossible to review each change and suggest better implementations

lua/entities/entity_fishing_rod/cl_init.lua

lua/entities/entity_fishing_rod/init.lua

lua/fishing_mod/catch/stove.lua

lua/fishing_mod/cl_init.lua

lua/fishing_mod/catch/stove.lua

Earu · 2020-09-05T01:05:26Z

@m4tsa mind reviewing these changes?

Earu

Overall there are a lot of inconsistencies namely:

variable casing all over the place
function casing all over the place
unneeded checks
unneeded parenthesis in ifs
short and bad variable names, your variable names should be descriptive and not less than 3 characters unless its something widely known such as pos, x, y etc...
lots of unneeded ifs that can be merged
inconsistencies in spaces between operators, you've added spaces in some places but not others

My review is non-exhaustive, I'm not going to add a comment for each individual issues because they appear A LOT of times in your changes.

lua/entities/entity_fishing_rod/cl_init.lua

lua/entities/entity_fishing_rod/init.lua

lua/fishing_mod/catch/stove.lua

lua/fishing_mod/cl_shop_menu.lua

lua/fishing_mod/sv_init.lua

lua/weapons/weapon_fishing_rod.lua

Myaats · 2020-09-05T10:26:20Z

@m4tsa mind reviewing these changes?

I'd rather just close the PR, currently it has more drawbacks and bad practices than benefits

Earu · 2020-09-05T10:52:27Z

It might be salvageable but a lot of work needs to be done on it @0x098 @m4tsa

Myaats · 2020-09-05T10:59:33Z

I suggest we just close this and require every feature on the list to be done in it's own PR that can be independently reviewed.

Earu · 2020-09-05T15:31:56Z

I don't see the point of creating a new PR for this, we're gonna lose all the relevant conversations, and what for? So it can look like a quick clean PR?

Earu · 2020-09-20T11:24:42Z

This looks better, @m4tsa please review these changes

Myaats · 2020-09-20T12:02:39Z

This looks better, @m4tsa please review these changes

Just because he actually tested his code and found some issues (there are probably more lurking around) instead of fixing the concerns posted does not make the PR any better in my eyes.

Earu · 2020-09-20T13:33:40Z

I'd argue that usually if you test more and fix more issues it makes a contribution better though (?)

fixing the concerns posted

Which are? The changes you've requested have been made and so have mine.

Myaats · 2020-09-20T13:39:35Z

I'd argue that usually if you test more and fix more issues it makes a contribution better though (?)

fixing the concerns posted

Which are? The changes you've requested have been made and so have mine.

I am not able to review changes individually to see if they make sense. So far I have only been able to look quickly through and look for obvious errors.

not enough testing v2

outdated

Earu

TL:DR

you have a lot of magic numbers that seem to be constants, make them actual constants and give them names
you have a lot of inconsistencies with your variable naming and spacing of arguments
you have unnecessary operations like *1

But all in all this is getting better

Earu · 2021-05-04T09:25:54Z

lua/fishing_mod/catch/stove.lua

 			surface.DrawRect(0, 0, x, y)
-			surface.DrawRect(3, 24, x-6, y-24-3)
+			surface.DrawRect(3, 24, x - 6, y - 24 - 3)


if your -3 is a constant please make it a variable such as CONSTANT = 3, if not then just put the actual number instead of doing unnecessary operations

Earu · 2021-05-04T09:27:13Z

lua/fishing_mod/cl_init.lua

+				text_height, text_width = surface.GetTextSize(string.Replace(string.Replace(data.text, "\t", ""), "\n", ""))
+				text_height, text_width = text_height + 8, text_width + 8
+				surface.DrawRect(xy.x - text_height / 2 - pad, xy.y - text_width / 2 - 1 - pad + 16, text_height + pad * 2, text_width + pad * 1)
+				surface.DrawRect(xy.x - text_height / 2 + 3 - pad, xy.y - text_width / 2 + 2 - pad + 16, text_height - 6 + pad * 2, text_width - 6 + pad * 1)


why do you multiply by 1?

Earu · 2021-05-04T09:27:40Z

lua/fishing_mod/cl_init.lua

+	if IsValid(LocalPlayer():GetActiveWeapon()) and LocalPlayer():GetActiveWeapon():GetClass() == "weapon_fishing_rod" then
+		surface.SetDrawColor(crosshair.r, crosshair.g, crosshair.b, crosshair.a)
+		local xy = chpos:ToScreen()
+		surface.DrawRect( xy.x, xy.y+5, 1, 10)


inconsistent spacing between numbers and operators

Earu · 2021-05-04T09:27:56Z

lua/fishing_mod/cl_init.lua

+local force_b = 1
+concommand.Add("fishing_mod_b_opens_always", function(ply, cmd, args)
+	if isnumber(tonumber(args[1])) then
+		force_b = math.Clamp(math.Round(args[1]),0,1)


inconsistent spacing between commas and arguments

Earu · 2021-05-04T09:28:52Z

lua/fishing_mod/cl_shop_menu.lua

+			if temp_data then
+				for k, v in pairs(temp_data) do
+					if k and v then
+						if #tostring(k) < 4 then return end


that looks really wonky

Earu · 2021-05-04T09:30:17Z

lua/fishing_mod/cl_shop_menu.lua

 		end
 		surface.DrawRect(0, 0, w, h)
 	end

-	baitsbutton:SetPos(3 + (xpx - 6) / 2, 24) -- baits shop start of conf
-	baitsbutton:SetSize((xpx - 6) / 2, 22)
+	baitsbutton:SetPos(3 + (xpx - 6) / 3, 24) -- baits shop start of conf


Not sure if this was part of the original code but when you do operations with "magic numbers" please make them constants such as CONSTANT = 3 it greatly helps reading

Earu · 2021-05-04T09:30:55Z

lua/fishing_mod/cl_shop_menu.lua

+		elseif not self.selected then
+			baitsbutton:SetColor(button_not_selected)
+		end
+		if baitsbutton:IsHovered() then


your variables have switched from PascalCase to snake_case to whateverthisis

Earu · 2021-05-04T09:31:41Z

lua/fishing_mod/cl_shop_menu.lua

+	self:SetVisible(false)
+	function self:Think()
+		x, y = self:GetParent():GetSize()
+		self:SetSize(x - 6, y - 3 - 46)


What is 3 ? Constant again ? Same comment as above

Earu · 2021-05-04T09:32:18Z

lua/fishing_mod/cl_shop_menu.lua

+	savebutton.DoClick = function()
+		if fishingmod.SaveUIColors then
+			fishingmod.SaveUIColors()
+			chat.AddText(Color(0, 255, 0, 255), "[Fishing Mod]", Color(255, 255, 255), ": Colors have been saved!")


if you're gonna re-use colors, please store them instead of re-creating them, creating tables in lua is intensive. This can be avoided

Earu · 2021-05-04T09:33:24Z

lua/fishing_mod/cl_shop_menu.lua

@@ -326,19 +658,19 @@ function PANEL:Init()
 end

 function PANEL:SetSale(multiplier)
-	self.percent = math.Round((multiplier*-1+1)*100)


why not just -multiplier ?

outdated approval

Python1320 · 2021-07-23T21:33:18Z

Is there still something major preventing merge? Considering this is the only fishingmod upgrade done for a long time I am inclined to just approve it for testing regardless of its prettyness. Easy to validate update requests get merged muuch faster.

outdated

Earu · 2021-07-24T09:11:21Z

@Python1320 for me not really, but I think @m4tsa still has things to say

gmod used to allow doing that

with a little seagull behaviour modification

Cynosphere previously requested changes Aug 16, 2020

View reviewed changes

lua/fishing_mod/cl_init.lua Outdated Show resolved Hide resolved

Myaats previously requested changes Aug 16, 2020

View reviewed changes

0x098 added 2 commits August 16, 2020 23:09

fixes

349fee9

Update cl_shop_menu.lua

8e069a9

0x098 requested a review from Myaats August 16, 2020 20:15

Earu requested changes Sep 5, 2020

View reviewed changes

not enough testing

545e03b

Earu previously approved these changes Sep 20, 2020

View reviewed changes

0x098 added 3 commits October 27, 2020 21:09

was bored

f22b6a0

not enough testing v2

yet some more minor fixes

d223de6

Merge branch 'master' of https://github.com/metastruct/fishingmod

70bb87b

Myaats requested a review from Earu May 4, 2021 08:55

Earu reviewed May 4, 2021

View reviewed changes

0x098 added 2 commits May 4, 2021 20:15

changes

80e5590

typo

1d99dc3

0x098 added 7 commits July 25, 2021 19:58

Update stove.lua

32f9662

scuffcode reduction

726c7b5

Merge branch 'Metastruct:master' into master

3055139

color issues

fbd0ac3

gmod used to allow doing that

Merge branch 'master' of https://github.com/0x098/fishingmod

2e7d839

seagull deterring and smol fixes

6802c4c

with a little seagull behaviour modification

https://github.com/Metastruct/fishingmod/issues/29

650fc23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new look is now customizable and minor behaviour fixes #26

new look is now customizable and minor behaviour fixes #26

0x098 commented Aug 16, 2020 •

edited

Loading

Myaats left a comment

Earu commented Sep 5, 2020

Earu left a comment •

edited

Loading

Myaats commented Sep 5, 2020

Earu commented Sep 5, 2020

Myaats commented Sep 5, 2020

Earu commented Sep 5, 2020

Earu commented Sep 20, 2020

Myaats commented Sep 20, 2020

Earu commented Sep 20, 2020 •

edited

Loading

Myaats commented Sep 20, 2020

Earu left a comment

Earu May 4, 2021

Earu May 4, 2021

Earu May 4, 2021

Earu May 4, 2021

Earu May 4, 2021

Earu May 4, 2021

Earu May 4, 2021

Earu May 4, 2021

Earu May 4, 2021

Earu May 4, 2021

Python1320 commented Jul 23, 2021

Earu commented Jul 24, 2021

new look is now customizable and minor behaviour fixes #26

Are you sure you want to change the base?

new look is now customizable and minor behaviour fixes #26

Conversation

0x098 commented Aug 16, 2020 • edited Loading

Myaats left a comment

Choose a reason for hiding this comment

Earu commented Sep 5, 2020

Earu left a comment • edited Loading

Choose a reason for hiding this comment

Myaats commented Sep 5, 2020

Earu commented Sep 5, 2020

Myaats commented Sep 5, 2020

Earu commented Sep 5, 2020

Earu commented Sep 20, 2020

Myaats commented Sep 20, 2020

Earu commented Sep 20, 2020 • edited Loading

Myaats commented Sep 20, 2020

Earu 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

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

Python1320 commented Jul 23, 2021

Earu commented Jul 24, 2021

0x098 commented Aug 16, 2020 •

edited

Loading

Earu left a comment •

edited

Loading

Earu commented Sep 20, 2020 •

edited

Loading