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

[WIP] add replacements for System.Numerics Vector3 / Quaternion #1638

Open
wants to merge 25 commits into
base: nagareyama
Choose a base branch
from

Conversation

0x53A
Copy link
Contributor

@0x53A 0x53A commented Nov 14, 2018

This adds replacements for System.Numerics Vector3 and Quaternion.

The reason for porting System.Numerics instead of binding an existing JS lib is that I want to run the same code serverside and clientside.

If you just need something for client side, i would still recomment to use a native js lib.

In the current WIP state this is just the subset I needed for my project, I plan to complete V3 and Q.

I do NOT plan to add everything from the package, i don't really care about Plane or Vector2, or Matrix_x_.

grafik

@alfonsogarciacaro
Copy link
Member

Looks good! Seems SIMD support has been dropped from JS standards so I guess we don't need to worry about that. We don't need to add everything from System.Numerics (we already have BigInteger for example) and vectors/quaternions should already be very useful. Just let me know if you need any help with this.

@0x53A
Copy link
Contributor Author

0x53A commented Nov 16, 2018

V3 / Q are the first replaced types to use member fields instead of member properties, so I had to modify FSharp2Fable a little bit.

When you have some time, could you review these changes please?

If that part is OK, then adding the remaining methods is just simple work.

// read a member field (Example: Vector3.X)
| BasicPatterns.ILFieldGet(Some callee, calleeType, fieldName)
when (Replacements.isILMemberFieldSupported (makeType com ctx.GenericArgs calleeType) fieldName) ->
let! callee = transformExprOpt com ctx (Some callee)
Copy link
Member

Choose a reason for hiding this comment

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

You can just use let! callee = transformExpr com ctx callee (same below for ILFieldSet).

@alfonsogarciacaro
Copy link
Member

Looks good! I just added a minor comment 👍

@alfonsogarciacaro alfonsogarciacaro force-pushed the master branch 3 times, most recently from dd1aeb3 to 027307b Compare February 21, 2019 22:20
@alfonsogarciacaro alfonsogarciacaro force-pushed the master branch 2 times, most recently from 13a9f4e to dbf433a Compare March 13, 2019 14:49
@alfonsogarciacaro alfonsogarciacaro force-pushed the master branch 2 times, most recently from 8c88154 to e2c26ad Compare April 1, 2019 22:19
@alfonsogarciacaro alfonsogarciacaro force-pushed the master branch 2 times, most recently from 9f9d44d to 6ae4cba Compare May 10, 2019 14:10
@0x53A
Copy link
Contributor Author

0x53A commented Aug 5, 2019

after overriding equality, it now fails with this weird error:

image

@0x53A
Copy link
Contributor Author

0x53A commented Aug 5, 2019

Ok, splitting the file into two so that there actually is a Vector3.js helped a bit, but equality still doesn't work:

image

@0x53A
Copy link
Contributor Author

0x53A commented Aug 5, 2019

It somehow still accessed the old file that I had split, a git clean later it now fails with

image

So some code expects a file Vector3.js, some other code expects a System.Numerics.js ( ఠ ͟ʖ ఠ)

@0x53A
Copy link
Contributor Author

0x53A commented Aug 5, 2019

It generated this weird code:

System.Numerics.js

"use strict";

Object.defineProperty(exports, "__esModule", {
  value: true
});
exports.Vector3$reflection = Vector3$reflection;
exports.Vector3$$$$002Ector$$Z7138B98C = Vector3$$$$002Ector$$Z7138B98C;
exports.Vector3$$$$002Ector$$Z69591B0C = Vector3$$$$002Ector$$Z69591B0C;

// ...

var _System = require("./System.Text");

var _Vector = _interopRequireDefault(require("./Vector3"));

var _Util = require("./Util");

// ...

Vector3.prototype.Equals = function (other$$1) {
  const this$$$8 = this;

  if (other$$1 instanceof _Vector.default) {
    const vother = other$$1;
    return Vector3$$Equals$$ZF6770AE(this$$$8, vother);
  } else {
    return false;
  }
};

// ...

Vector3.prototype.CompareTo = function (other$$2) {
  const this$$$10 = this;

  if ((0, _Util.equals)(other$$2, null)) {
    return (0, _String.toFail)((0, _String.printf)("other is null")) | 0;
  } else if (other$$2 instanceof _Vector.default) {
    const vother$$1 = other$$2;
    return (0, _Util.compareArrays)([this$$$10.X, this$$$10.Y, this$$$10.Z], [vother$$1.X, vother$$1.Y, vother$$1.Z]) | 0;
  } else {
    return (0, _String.toFail)((0, _String.printf)("invalid type, expected 'other' to be a Vector, but it is '%O'"))(_Reflection.obj) | 0;
  }
};

// ...

Note the var _Vector = _interopRequireDefault(require("./Vector3")); and the usages of _Vector.default.

so the typechecks are the issue.

Or rather the fact that structs should have an autogenerated parameterless ctor, but fable doesn't seem to generate one.

@alfonsogarciacaro
Copy link
Member

Hmm, I'd have to look at the PR more carefully to find the problem but I see you're using bclType for the Vector3 replacement. This was create to automatically find the JS code using the namespace of the type as the file name and then resolving the mangling of the class and member names.

Also, numeric operations are tricky because they must be resolved in different places. You probably need to add the new BclQuaternion and BclVector3 to the following functions in Replacements.fs: applyOp, equals, compare, compareIf, getZero, getOne, defaultof

@alfonsogarciacaro alfonsogarciacaro force-pushed the master branch 11 times, most recently from 7fe56ba to e549bb1 Compare September 10, 2019 08:36
@alfonsogarciacaro alfonsogarciacaro changed the base branch from master to nagareyama December 3, 2020 06:25
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