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

RFE: Fraction decompilation - more weight to common divisors? #1459

Open
tamlin-mike opened this issue Mar 18, 2019 · 4 comments
Open

RFE: Fraction decompilation - more weight to common divisors? #1459

tamlin-mike opened this issue Mar 18, 2019 · 4 comments
Labels
C# Decompiler The decompiler engine itself Enhancement Areas for improvement Help Wanted

Comments

@tamlin-mike
Copy link

I really liked the introduction of fractions instead of numbers with long decimal sequences. It cought me by surprise and put a smile on my face. That feature is the very definition of quality. Really good job!

But, there's always room for improvement. :-)

I think it would make sense to give preference to the following divisors:
2^n for n in (8,10,12-16, 20)
2^n-1 for n in (8,10,12,15,16)

Some rationale:
255 (2^8-1) is very common when converting (u)byte (f.ex. RGB) <-> normalized float.
2^10 and 2^20 is common when converting larger numbers to computer-K and -M.
I saw a long color initialization sequence where the decompiler preferred 51 as divisor, where 255 would have been more obvious (example below).

Coming to think of it, should negative numbers become handled, maybe even 2^7 and 2^7-1 should be considered.

I have also seen code normalizing values in f.ex the ranges +/- 4K, 16K, 32K and 64K, so maybe negative numbers should be considered too (at least for a/this small subset of potential divisors)?

I believe that would allow better/nicer decompilation of some code than the following example of current decompiler output (all three should preferrably be divisor 255f):
0.882352948f, 40f / 51f, 0.5882353f.

Again, good job!

@tamlin-mike
Copy link
Author

For your consideration.

While on the subject...

2*PI is a common constant used in some types of applications.

If a type is already using something from the namespace UnityEngine,

namespace UnityEngine {
public struct Mathf
{
	public const float PI = 3.14159274f;
	public const float Infinity = float.PositiveInfinity;
	public const float NegativeInfinity = float.NegativeInfinity;
	public const float Deg2Rad = 0.0174532924f;
	public const float Rad2Deg = 57.29578f;
	public static readonly float Epsilon;
}}

Should UnityEngine(.Mathf?) not already be included, perhaps deg<->rad constants could be added to the "specials" list and become inlined (180f/PI) etc?

Just some ideas.

@tamlin-mike
Copy link
Author

Sometimes "less is more". :-)

float num6 = 0.166666672f; // pre fractions ("1f / 6f" would be the obvious)
float num6 = 355f / (678f * (float)Math.PI); // Current. :-)

@siegfriedpammer
Copy link
Member

If you have the time and are interested, it would be nice, if you could take a look at the code yourself and provide some improvements as a pull request. 😄 The relevant code lives in TypeSystemAstBuilder, entry-point is the ConvertFloatingPointLiteral method. If you have any questions regarding the existing code, please feel free to ask! (Please do not forget to extend the WellKnownConstants test case.) Thank you very much!

@tamlin-mike
Copy link
Author

Another improvement (adding it here so it's not forgotten) would be to identify negative constants too.
Example: float.Epsilon is currently identified, but -float.Epsilon is not.

@siegfriedpammer siegfriedpammer added C# Decompiler The decompiler engine itself Enhancement Areas for improvement Help Wanted labels Jul 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# Decompiler The decompiler engine itself Enhancement Areas for improvement Help Wanted
Projects
None yet
Development

No branches or pull requests

2 participants