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

Make "decimal" functions more consistent #21425

Merged
merged 1 commit into from
May 1, 2019

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Aug 25, 2018

Previously, "decimals()" in GDScript and "Decimals()" in C# had different behavior. I have renamed the method to "step_decimals()". It was already called that internally. C# is now consistent with core.

@aaronfranke aaronfranke force-pushed the decimal-functions branch 3 times, most recently from 022a588 to d4f6e7c Compare August 26, 2018 02:08
@akien-mga akien-mga added this to the 3.1 milestone Aug 27, 2018
@akien-mga
Copy link
Member

I don't think decimals is the right name for a function that gives the fractional part of a number - the plural doesn't make sense, it should just be decimal.

That said, we're not keen on breaking compatibility unless there is a strong incentive, so I'm not convinced by these changes.

@Chaosus Chaosus modified the milestones: 3.1, 3.2 Aug 28, 2018
@CptPotato
Copy link
Contributor

I find the name decimal unfitting for a function that gives you the fractional part of a (decimal) number.
How about naming it frac() or fract() as used in shader languages?

@groud
Copy link
Member

groud commented Aug 28, 2018

I don't even know why we need this function, which can easily be solved by fmod(myvar, 1.0)

@CptPotato
Copy link
Contributor

You're right, though modulo is a bit slower.

@aaronfranke
Copy link
Member Author

aaronfranke commented Aug 28, 2018

Then should I remove the new "decimals()" functionality and instead leave it pointing to "step_decimals" like it was before? That would make it not break compatibility.

EDIT: Done, does not break compatibility anymore

@akien-mga
Copy link
Member

IMO it's not worth renaming decimals to step_decimals at this stage, but we can add it in #16863 to the list of methods that we want to rename next time we break compatibility.

@akien-mga
Copy link
Member

No longer breaks compat.

If we're deprecating decimals to replace it by step_decimals, I'd add something like this to GDScriptFunctions::call and Expression::exec_func:

ERR_EXPLAIN("GDScript method 'decimals' is deprecated and replaced by 'step_decimals', please update your code accordingly.");
WARN_DEPRECATED

(and then still fallback to step_decimals handling so that it works even if deprecated).

About the addition of DecimalsCount in C#, that's up to @neikeq. I've already mentioned how I feel about adding C#-only features when there's no reason why C# users would need them more than GDScript users. And the whole point of using C# in the first place is to have access to the huge ecosystem of C# libraries, many of which probably cover such method already so that we don't have to reimplement the wheel in core (beyond the compat layer with GDScript).

@akien-mga
Copy link
Member

Since 74eefda56 is independent of the renaming proposal, I'd suggest to put it in its own PR so that I can merge it as a bugfix.

@aaronfranke
Copy link
Member Author

Here it is then: #22251

@aaronfranke
Copy link
Member Author

aaronfranke commented Oct 26, 2018

@akien-mga I've added the deprecation warnings as you suggested.

And to clarify, as I mentioned in #22251, the only reason DecimalCount exists is because the C# code had this functionality already, but under the "Decimals" name. It can be removed if requested (and/or in the future & CC @neikeq), and I don't need it personally, I'm just trying to keep the existing functionality.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't have a strong opinion on this matter but it's being deprecated the right way, so that's fine with me to merge. If anyone is against it, speak now or never :)

In GDScript, rename "decimals" to "step_decimals". In C#, add "StepDecimals", but keep the old functionality in a method called "DecimalCount".
@aaronfranke
Copy link
Member Author

So, just to clarify the justification for this PR:

decimals was a confusing name for this method, to the point that I was confused by it, and the previous C# implementation was wrong. So it's been renamed, and C# is now consistent with Core.

@akien-mga akien-mga merged commit 8afc9c3 into godotengine:master May 1, 2019
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the decimal-functions branch May 1, 2019 08:20
akien-mga added a commit to akien-mga/godot that referenced this pull request Feb 12, 2020
Replaced by 'step_decimals' in 3.2 via godotengine#21425.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants