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

Fix issue with onSuccess with bad argument #231

Merged
merged 12 commits into from
Dec 3, 2022
Merged

Conversation

kalwalt
Copy link
Member

@kalwalt kalwalt commented Nov 17, 2022

For more infos see issue #230
The code works as usual because the distribution file is not affected but need to be tested with ARnft or other Typescript project.
I would also remove as much as possible all any in the code and give more consistent type checking.
For testing with npm use this, replacing <hash_commit> with the actual hash commit number:

"@webarkit/jsartoolkit-nft": "https://github.com/webarkit/jsartoolkitNFT.git#<hash-commit>",

@kalwalt kalwalt added bug Something isn't working enhancement New feature or request Typescript labels Nov 17, 2022
@kalwalt kalwalt self-assigned this Nov 17, 2022
@@ -272,7 +269,7 @@ export default class ARToolkitNFT {
onError2(errorNumber);
};

let Ids: any = [];
let Ids: [{id: number}];
Copy link
Member Author

@kalwalt kalwalt Nov 17, 2022

Choose a reason for hiding this comment

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

This is correct but it is a silent error, when the marker is loaded you will receive this kind of error:

aa1e448a-43ef-43be-881e-d5c11c836675:1 Error in loading marker on Worker TypeError: Cannot read properties of undefined (reading 'push')
    at aa1e448a-43ef-43be-881e-d5c11c836675:1:748671
    at Array.forEach (<anonymous>)
    at A.value (aa1e448a-43ef-43be-881e-d5c11c836675:1:748283)
    at A.<anonymous> (aa1e448a-43ef-43be-881e-d5c11c836675:1:755365)
    at G (aa1e448a-43ef-43be-881e-d5c11c836675:1:928)
    at Generator.<anonymous> (aa1e448a-43ef-43be-881e-d5c11c836675:1:2283)
    at Generator.next (aa1e448a-43ef-43be-881e-d5c11c836675:1:1291)
    at A (aa1e448a-43ef-43be-881e-d5c11c836675:1:8095)
    at i (aa1e448a-43ef-43be-881e-d5c11c836675:1:8298)
    at aa1e448a-43ef-43be-881e-d5c11c836675:1:8357
(anonymous) @ aa1e448a-43ef-43be-881e-d5c11c836675:1
Promise.catch (async)
(anonymous) @ aa1e448a-43ef-43be-881e-d5c11c836675:1
Promise.then (async)
D @ aa1e448a-43ef-43be-881e-d5c11c836675:1
C.onmessage @ aa1e448a-43ef-43be-881e-d5c11c836675:1

src/ARToolkitNFT.ts Outdated Show resolved Hide resolved
src/ARToolkitNFT_simd.ts Outdated Show resolved Hide resolved
@kalwalt
Copy link
Member Author

kalwalt commented Nov 17, 2022

This commit fixed the above issue but i'm not sure that the final result is an Arrays of ids like said [0, 1, 2...]

@kalwalt
Copy link
Member Author

kalwalt commented Nov 17, 2022

This commit fixed the above issue but i'm not sure that the final result is an Arrays of ids like said [0, 1, 2...]

No i was wrong, because not looking at the right example.
@antokhio let me know if this solved the issue. 😄

@kalwalt
Copy link
Member Author

kalwalt commented Nov 27, 2022

@antokhio have you tested this? If it's ok i will merge in the next days if i can.

@kalwalt
Copy link
Member Author

kalwalt commented Dec 1, 2022

i found that loadNFTMarkers the onError parameter should have one argument as number instead now is empty:

async loadNFTMarkers(
urlOrData: Array<string>,
onSuccess: (ids: number[]) => void,
onError: () => void
) {
let nft = await this.artoolkitNFT.addNFTMarkers(
this.id,
urlOrData,
(ids) => {
this.nftMarkerCount += ids.length;
onSuccess(ids);
},
onError
);
return nft;
}

it should reflect the underlying addNFTMarkers:
public addNFTMarkers(
arId: number,
urls: Array<string | Array<string>>,
callback: (filename: number[]) => void,
onError2: (errorNumber: number) => void
): Array<number> {

@kalwalt
Copy link
Member Author

kalwalt commented Dec 2, 2022

I think it can be merged i tested it and i cannot find any issues. If i can i will merge very soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request Typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant