Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Small Fixes :) #144

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Small Fixes :) #144

wants to merge 6 commits into from

Conversation

Dentrax
Copy link

@Dentrax Dentrax commented Jul 22, 2017

It includes some simple bug fixes & additionals.

Good Luck :)

Demigiant and others added 5 commits March 22, 2015 13:47
Fixed an issue with Path.cs possible 'Division by zero' error
Fixed compiler warning error 'Possible null reference'
Added 'Comparisons should be use 'Epsilon' value. Like : Math.Abs(A - B) < Epsilon' notice :)
Fixed operator priority with missing parentheses
A forgotten switch-case was added
@Dentrax Dentrax closed this Jul 22, 2017
@Dentrax
Copy link
Author

Dentrax commented Jul 22, 2017

Checked, everything seems up-to-date with 'develop' branch.. There is no critical situation.

@Dentrax Dentrax reopened this Jul 22, 2017
@Demigiant
Copy link
Owner

Hey Furkan! That is great :) Give me a couple days to review the changes though, I'm in momentary panic now (annual taxes period here, much more than expected) and can't focus on this :P

@Dentrax
Copy link
Author

Dentrax commented Jul 24, 2017

Taxes are important. :) Thank you for your interest. We love Dotween. <3

@Demigiant
Copy link
Owner

Demigiant commented Jul 27, 2017

Here I am (damn taxes! I mean, I totally approve taxes, except when they ask me to pay more than what I earned by jove :P).

Let's talk about the pull request, and thanks for the nice words :)

EaseManager.cs
I know I should use Epsilon, but that makes things slightly slower, plus those eases are standard ones created by Robert Penner and never had problems with Epsilon, so I'd tend to leave them like that for better performance :P

TweenManager.cs
All the changes you made are great but are actually already in the most recent code. I don't know if github went crazy (because at line 354 there's also the exact same comment I wrote), I suddenly forgot how to read pull requests correctly, or if we just worked on paired minds. The only change that would be "new" is line 393, but that's actually just coding style so I wouldn't touch it (I never use parenthesis to group elements between ORs).

Path.cs
I have doubts about this, because of these two points:

  • Wrapping all of that inside an IF would also prevent the first 0% point to be drawn
  • It's true that using an IF would prevent the calculations for the first point, which are unnecessary. But that IF would be useful only once, and it would add a check to each pass of the loop. Considering that there might be a LOT of subdivisions (and usually there are), I think it's more efficient to have 1 additional calculation per loop instead of N additional checks per pass.

Vector3ArrayPlugin.cs
Assigning a value to res is unnecessary (and you shouldn't get a warning in that case), because we have a switch with a default case that takes care of that anyway. This is coding style, and I personally prefer it like that, because it tells me "this variable is surely gonna be assigned later" :P

This is my humble opinion, but I don't pretende to be right, so let me know what you think and let's discuss :)

Cheers!

@Dentrax
Copy link
Author

Dentrax commented Aug 1, 2017

Hey ! :)

(P.S : Sorry for my awesome english (: )
I thought a bit and you are really right. I think this is a case of experience. :) But I still have concerns about a Path.cs (zero-division-error). You are completely right about : "That IF would be useful only once, and it would add a check to each pass of the loop". It is my fool. I think It should be :
(if((float)gizmosSubdivisions != 0)
for...)

Maybe I can do better things next time! :) Thank you for your time to me!
Best regards...

@Dentrax Dentrax closed this Aug 1, 2017
@Demigiant
Copy link
Owner

Demigiant commented Aug 1, 2017

Nono are you joking, you did great! I love that you suggested changes, and this was an interesting chat :)

About the division by 0, I thought about that. That would happen only if a user decides to pass a path resolution of 0. Since that makes no sense, I'm happy to throw him an error. But you're right, I should maybe capture that instead and just set the resolution to 1 automatically. I will change this! But what do you think would be best? Log a clear-er error that says "resolution must be at least 1", or capture it and set it to 1 behind the scenes?

@Demigiant Demigiant reopened this Aug 1, 2017
@Dentrax
Copy link
Author

Dentrax commented Aug 4, 2017

Thank you so much ! :) I think the best way is you should threw an exception. If "safemode" enabled, you could set it to 1.

Best regards... :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants