-
Notifications
You must be signed in to change notification settings - Fork 960
Testing type definitions (Typescript) #506
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
Conversation
@@ -36,7 +42,7 @@ const MarkersAreaControls = function (arToolkitContext, object3d, parameters) { | |||
}; | |||
|
|||
MarkersAreaControls.prototype = Object.create(ArBaseControls.prototype); | |||
MarkersAreaControls.prototype.constructor = MarkersAreaControls; | |||
//MarkersAreaControls.prototype.constructor = MarkersAreaControls; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickw1 this is odd. I think this is an error in the code design. Actually the constructor of the class is created at line 6:
const MarkersAreaControls = function (arToolkitContext, object3d, parameters) {
var _this = this;
ArBaseControls.call(this, object3d);
I have to comment out this line because tsc
compiler will create another constructor as a property function instead with a resulting error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably we should convert the code to really ES6 standard before adding type defs.
so converting the above class to:
class MarkersAreaControls extends ArBaseControl {
constructor(arToolkitContext, object3d, parameters) {
// other code here
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kalwalt I'm not familiar with this code, but I would strongly support using ES6 classes over older methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that AR.js can be improved a lot in terms of readability if rewritten using ES6 classes. Not sure if this task it can be done before or after adding type definitions. This PR is a kind of test at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway removing that line has no side effect, i recompiled the libs and tested most of the examples with success. I think this was an old bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading in the mdn documentation prototype.constructor is actually the constructor of the created object (class). So not a mistake but maybe it's not needed. Anyway i will continue to convert the code ot Typescript and a more modern standard.
@@ -99,7 +106,7 @@ const MarkerControls = function (context, object3d, parameters) { | |||
}; | |||
|
|||
MarkerControls.prototype = Object.create(ArBaseControls.prototype); | |||
MarkerControls.prototype.constructor = MarkerControls; | |||
//MarkerControls.prototype.constructor = MarkerControls; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as in comment https://github.com/AR-js-org/AR.js/pull/506/files#r1071485190
Hey. As mentioned previously when installing the package, and setting up the project it will only work if you click the "VR" button on the video. To test this just do the following:
You can see what I am scanning with my webcam. (Also notice the "fullscreen" button at the lower right corner) Pressing that "fullscreen" button shows the T rex Btw. I have not done anything other than using the Vue quick start guide. |
@commentatorboy i forgot to add |
I'm testing type definitons in this my repository: https://github.com/kalwalt/AR.js-typescript This is what i found:
import ArMultiMarkerControls from "./markers-area/arjs-markersareacontrols";
import ArMultiMakersLearning from "./markers-area/arjs-markersarealearning";
import ArMultiMarkerUtils from "./markers-area/arjs-markersareautils";
export {
ArMarkerControls,
ArMarkerHelper,
ArSmoothedControls,
ArToolkitContext,
ArToolkitProfile,
ArToolkitSource,
ArMultiMarkerControls,
ArMultiMakersLearning,
ArMultiMarkerUtils,
};
//# sourceMappingURL=index-threex.d.ts.map but should be instead: import ArMarkerControls from "./threex/arjs-markercontrols";
import ArMarkerHelper from "./threex/threex-armarkerhelper";
import ArSmoothedControls from "./threex/threex-arsmoothedcontrols";
import ArToolkitContext from "./threex/arjs-context";
import ArToolkitProfile from "./threex/arjs-profile";
import ArToolkitSource from "./threex/arjs-source";
import ArMultiMarkerControls from "./markers-area/arjs-markersareacontrols";
import ArMultiMakersLearning from "./markers-area/arjs-markersarealearning";
import ArMultiMarkerUtils from "./markers-area/arjs-markersareautils";
export {
ArMarkerControls,
ArMarkerHelper,
ArSmoothedControls,
ArToolkitContext,
ArToolkitProfile,
ArToolkitSource,
ArMultiMarkerControls,
ArMultiMakersLearning,
ArMultiMarkerUtils,
};
//# sourceMappingURL=index-threex.d.ts.map not all type definitions are created by the tsc compiler, don't know why...
|
Now I am not entirely into typescript, so I might be not be of too much help here 😓 But I have some questions though It is not exported in https://github.com/AR-js-org/AR.js/blob/master/three.js/src/threex/arjs-markercontrols.js Where is this "mapping" defined? I agree on point 4. |
I'm not a Typescript guruu but i think i can develop a Typescript version with help of the AR.js community!
This because the old implementation doesn't adopt the new ES6 syntax when it was developed ( that come later... take a look at the old code here: https://github.com/AR-js-org/AR.js/blob/aaad9847f67a4738b00724c38d9f501d78e0a8af/three.js/src/threex/threex-armarkercontrols.js var ARjs = ARjs || {}
var THREEx = THREEx || {}
ARjs.MarkerControls = THREEx.ArMarkerControls = function(context, object3d, parameters){
var _this = this
// other code here...
} I think we should go over this and think in a more modern approach. As i said, and i remark this, we should start to redesign the code with a modular concept in mind. In 2023 we all the tools for doing this. We could start with a new AR.js-threex and AR.js-base modules, that can be integrated into AR.js. |
And more: With Typescript we can create two namespaces AR.js and THREEx and create inside the two, all the classes. // inside ArMarkerControls.ts
import { IContext, IObject3d, IParams } from 'interfaces/CommonInterfaces'
namespace THREEx {
class ArMarkerControls {
constructor(context: IContext, object3d: IObject3D, parameters: IParams) {
}
}
}
// inside MarkerControls.ts
import ArMarkerControls from '@ar-js-org/arjs-threex'
namespace ARjs {
class MarkerControls extends ArMarkerControls {
constructor(context: any, object3d: any, parameters: any) {
super(context, object3d, parameters)
}
}
} |
Agreed. |
Working on a typescript version this my repo ARj.s-threex. There is a basic example, but not fully working yet. I hope to have more time next week to improve the code. |
In regards of this PR i think i will not merge. I think there are many issues in it to be solved. |
@kalwalt sounds the best plan if so. |
@commentatorboy have you tried AR.js-threejs? |
What kind of change does this PR introduce?
This PR will try to add Type definitions to be used in a Typescript project.
Can it be referenced to an Issue? If so what is the issue # ?
see #402
How can we test it?
No test provided yet, but i will create a Typescript project for testing.
Summary
Does this PR introduce a breaking change?
It shouldn't create any issue.
Please TEST your PR before proposing it. Specify here what device you have used for tests, version of OS and version of Browser
I'm testing types defs in this my repository https://github.com/kalwalt/AR.js-typescript
Other information