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

AddTicks not work correctly #53

Open
Ali1Jamal opened this issue Sep 28, 2021 · 11 comments
Open

AddTicks not work correctly #53

Ali1Jamal opened this issue Sep 28, 2021 · 11 comments

Comments

@Ali1Jamal
Copy link

Ali1Jamal commented Sep 28, 2021

add ticks not work correctly in this simple :
https://deck.net/d8843f388823586dca2a2b94bbbd6af1
Expected Result
2019-12-01T00:00:00.0000000
2019-11-30T23:59:59.9990000
2019-11-30T23:59:59.9990000
Actual Result
2019-12-01T00:00:00.0000000
2019-12-01T00:00:00.0000000
2019-11-30T23:59:59.9990000
i think to fix this problem its to replace:
https://github.com/theolivenbaum/h5/blob/d9c8a733e6b55584c3bd37b1f461ae9378882550/H5/H5/shared/System/DateTime.cs#L454
with is :
return new DateTime((ulong)(ticks + value));
or with this:
return new DateTime((long)(ticks + value));

@theolivenbaum
Copy link
Collaborator

Unfortunately that DateTime file is not used and is only kept in the repo for reference, you can see how it's excluded from the H5.csproj file. The actual DateTime code is under the Date.js file, and the other DateTime.cs that simply uses the Template function to redirect methods to the javascript implementation

  <ItemGroup>
    <Compile Remove="shared\System\DateTime.cs" />
    <Compile Remove="shared\System\Globalization\InternalGlobalizationHelper.cs" />
    <Compile Remove="shared\System\Reflection\MethodInfo.cs" />
    <Compile Remove="shared\System\Resources\RuntimeResourceSet.cs" />
  </ItemGroup>
/// <summary>
/// Returns a new DateTime that adds the specified number of ticks to the value of this instance.
/// </summary>
/// <param name="value">A number of 100-nanosecond ticks. The value parameter can be positive or negative.</param>
/// <returns>An object whose value is the sum of the date and time represented by this instance and the time represented by value.</returns>
[H5.Template("System.DateTime.addTicks({this}, {0})")]
public extern DateTime AddTicks(long value);
addTicks: function (d, v) {
    v = System.Int64.is64Bit(v) ? v : System.Int64(v);

    var dt = new Date(d.getTime()),
        ticks = this.getTicks(d).add(v);

    dt.setMilliseconds(dt.getMilliseconds() + v.div(10000).toNumber())
    dt.ticks = ticks;
    dt.kind = (d.kind !== undefined) ? d.kind : 0;

    return dt;
},

@theolivenbaum
Copy link
Collaborator

The issue seems to be on the internal Date representation:
https://deck.net/b67912ba0b6a60fd45e18cb7a628c38e

public class Program
{
    public static void Main()
    {
        DateTime time1 = new DateTime(2019, 12, 1);
        DateTime time2 = time1.AddTicks(-1);
        DateTime time3 = new DateTime(time1.Ticks - 1);
        
        Console.WriteLine(time1.Ticks);
        Console.WriteLine(time1.ToString("O"));
        
        Console.WriteLine(time2.Ticks);
        Console.WriteLine(time2.ToString("O"));
        
        Console.WriteLine(time3.Ticks);
        Console.WriteLine(time3.ToString("O"));
    }
}

prints:

637107552000000000
2019-12-01T00:00:00.0000000

637107551999999999 // correct
2019-12-01T00:00:00.0000000 // wrong

637107551999999999 // correct
2019-11-30T23:59:59.9990000 //wrong

@theolivenbaum
Copy link
Collaborator

It seems to be a precision issue. If we instead subtract 10000 ticks (i.e. 1ms), the results are matching:

https://deck.net/83725242e22d749f012002d409cf330c

637107552000000000
2019-12-01T00:00:00.0000000

637107551999900000
2019-11-30T23:59:59.9900000

637107551999900000
2019-11-30T23:59:59.9900000

@theolivenbaum
Copy link
Collaborator

Compared to C#:

https://dotnetfiddle.net/WG0BO9

Subtracting one tick:

637107552000000000
2019-12-01T00:00:00.0000000

637107551999999999
2019-11-30T23:59:59.9999999

637107551999999999
2019-11-30T23:59:59.9999999

You can see the extra precision there

@Ali1Jamal
Copy link
Author

Unfortunately that DateTime file is not used and is only kept in the repo for reference, you can see how it's excluded from the H5.csproj file. The actual DateTime code is under the Date.js file, and the other DateTime.cs that simply uses the Template function to redirect methods to the javascript implementation

  <ItemGroup>
    <Compile Remove="shared\System\DateTime.cs" />
    <Compile Remove="shared\System\Globalization\InternalGlobalizationHelper.cs" />
    <Compile Remove="shared\System\Reflection\MethodInfo.cs" />
    <Compile Remove="shared\System\Resources\RuntimeResourceSet.cs" />
  </ItemGroup>
/// <summary>
/// Returns a new DateTime that adds the specified number of ticks to the value of this instance.
/// </summary>
/// <param name="value">A number of 100-nanosecond ticks. The value parameter can be positive or negative.</param>
/// <returns>An object whose value is the sum of the date and time represented by this instance and the time represented by value.</returns>
[H5.Template("System.DateTime.addTicks({this}, {0})")]
public extern DateTime AddTicks(long value);
addTicks: function (d, v) {
    v = System.Int64.is64Bit(v) ? v : System.Int64(v);

    var dt = new Date(d.getTime()),
        ticks = this.getTicks(d).add(v);

    dt.setMilliseconds(dt.getMilliseconds() + v.div(10000).toNumber())
    dt.ticks = ticks;
    dt.kind = (d.kind !== undefined) ? d.kind : 0;

    return dt;
},

oh i'm didn't know this info ... nice Investigation

@Ali1Jamal
Copy link
Author

The issue seems to be on the internal Date representation: https://deck.net/b67912ba0b6a60fd45e18cb7a628c38e

public class Program
{
    public static void Main()
    {
        DateTime time1 = new DateTime(2019, 12, 1);
        DateTime time2 = time1.AddTicks(-1);
        DateTime time3 = new DateTime(time1.Ticks - 1);
        
        Console.WriteLine(time1.Ticks);
        Console.WriteLine(time1.ToString("O"));
        
        Console.WriteLine(time2.Ticks);
        Console.WriteLine(time2.ToString("O"));
        
        Console.WriteLine(time3.Ticks);
        Console.WriteLine(time3.ToString("O"));
    }
}

prints:

637107552000000000 2019-12-01T00:00:00.0000000

637107551999999999 // correct 2019-12-01T00:00:00.0000000 // wrong

637107551999999999 // correct 2019-11-30T23:59:59.9990000 //wrong

yes its seems to be .

@ghost
Copy link

ghost commented Oct 8, 2021

It seems to be a precision issue. If we instead subtract 10000 ticks (i.e. 1ms), the results are matching:

https://deck.net/83725242e22d749f012002d409cf330c

637107552000000000 2019-12-01T00:00:00.0000000

637107551999900000 2019-11-30T23:59:59.9900000

637107551999900000 2019-11-30T23:59:59.9900000

There are 2 problems there. First of all the following code is wrong:

v.div(10000).toNumber()

v is Int64, and divide in 10000 does not make sense, integer result for -1 / 10000 is zero. Adding zero does nothing ))

But replacement from

dt.setMilliseconds(dt.getMilliseconds() + v.div(10000).toNumber())

to

dt.setMilliseconds(dt.getMilliseconds() + v.toNumber() / 10000);

// ";" in the end is also required

will not work... due to limitations of Date object itself. The maximal precision is 1 ms (1/1000 sec):

(new Date(1633689504074)).getMilliseconds()
74
(new Date(1633689504075)).getMilliseconds()
75

So to make it really compatible with .NET platform we need to replace Date.js by transpiled version of DateTime.cs, I expect significant amount of work here, but finally it should be made. Alternatively we need to use some date library inside Date.js, but I expect such behavior more problematic in case of supporting Calendar object in H5.

Not minified Date.js takes around 50 KB, so from point of view of size it should not be a problem.

@theolivenbaum
Copy link
Collaborator

@hardhub I think the reason they didn't implement DateTime as is in the original Bridge is due to different calendars and the general complexity around supporting dates. I doubt this is going to be simple, as the .NET runtime uses information from the OS which would not be accessible to the javascript world without using their Date data type.

Unless we have someone that can dig deep into this, I would prefer to focus any work here on fixing the obvious bugs with time zones and such.

@ghost
Copy link

ghost commented Oct 8, 2021

@hardhub I think the reason they didn't implement DateTime as is in the original Bridge is due to different calendars and the general complexity around supporting dates. I doubt this is going to be simple, as the .NET runtime uses information from the OS which would not be accessible to the javascript world without using their Date data type.

Unless we have someone that can dig deep into this, I would prefer to focus any work here on fixing the obvious bugs with time zones and such.

But AddTicks does not work. It disallow to create ranges. For example you need to create period of current week. The end date should be AddDays(+7).AddTicks(-1). So it is last possible time of desired week. But in case of wrong implementation of AddTicks it is not possible. And if it is cross-compiled code then all is even worse.

Probably we can replace js code of addTicks by code of constructor used here:

DateTime time3 = new DateTime(time1.Ticks - 1);

I will check the source code.

@ghost
Copy link

ghost commented Oct 8, 2021

@theolivenbaum I think you can use create$2 version inside AddTicks. Please use the following code (and I will not fork the repo for the moment).

addTicks: function (d, v) {
                v = System.Int64.is64Bit(v) ? v : System.Int64(v);

                var ticks = this.getTicks(d).add(v);
                var kind = d.kind;

                if (ticks.lt(this.TicksPerDay)) {
                    d = new Date(0);
                    d.setMilliseconds(d.getMilliseconds() + this.$getTzOffset(d).div(10000).toNumber());
                    d.setFullYear(1);
                } else {
                    d = new Date(ticks.sub(this.$getMinOffset()).div(10000).toNumber());

                    if (kind !== 1) {
                        d.setTime(d.getTime() + (d.getTimezoneOffset() * 60000));
                    }
                }

                d.kind = (kind !== undefined) ? kind : 0;
                d.ticks = ticks;

                return d;
            },

I have adjusted it a bit for needs of AddTicks. And it shows more correct result.

2019-12-01T00:00:00.0000000
2019-11-30T23:59:59.9990000
2019-11-30T23:59:59.9990000

Of course it is not 999 9999, but at least the behaviour is equal to other DateTime logic and bug is solved.

@theolivenbaum
Copy link
Collaborator

Looks good, I'll add over the weekend when I've some time, thanks!

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

No branches or pull requests

2 participants