webarkit / jsartoolkitNFT

jsartolkitNFT is a smaller version of jsartoolkit5 with only NFT support
GNU Lesser General Public License v3.0
131 stars 25 forks source link

loadNFTMarkers has incorrect onSuccess signature #230

Closed antokhio closed 1 year ago

antokhio commented 1 year ago

loadNFTMarkers has incorrect return signature

(method) ARControllerNFT.loadNFTMarkers(urlOrData: string[], onSuccess: (ids: number) => void, onError: () => void): Promise<[{
    id: number;
}]>
onSuccess: (ids: number) should be onSuccess: (ids: number[])

currently have to convert it to unknow first:

await controller?.loadNFTMarkers(markers, (ids:unknown) => {...}, ... )
kalwalt commented 1 year ago

Hi @antokhio have you a piece of code to test? currently with ARnft (see Worker.ts) i never had this issue https://github.com/webarkit/ARnft/blob/bf748f29af0ce694c2f8730115468a113fb884a4/src/Worker.ts#L104-L116

       ar.loadNFTMarkers(nftMarkerUrls, (id: number) => {
            let marker = ar.getNFTData(ar.id, 0);
            ctx.postMessage({ type: "markerInfos", marker: marker });
            ar.trackNFTMarkerId(id);
            console.log("loadNFTMarker -> ", id);
            console.log(id);
            ctx.postMessage({ type: "endLoading", end: true });
        }).catch((err: any) => {
            console.error("Error in loading marker on Worker", err);
        });

        ctx.postMessage({ type: "loaded", proj: JSON.stringify(cameraMatrix) });
    };

I think jsartoolkitNFT need some improves in type checking see for example the LoadNFTMarkers https://github.com/webarkit/jsartoolkitNFT/blob/9277579a055a20ec1bee1e8f9437b273ea9806ed/src/ARControllerNFT.ts#L704-L719 at line 712

(ids: any) => {
...
}

I think this is really odd...

antokhio commented 1 year ago

@kalwalt here is WIP project, https://github.com/antokhio/react-three-jsartoolkitnft/blob/main/src/workers/arnft.worker.ts

Just to note, trying to make it an npm package, but i have issues with web worker bundling i can't sort out for a week or so ;) The code should be working if you copy the worker js from npm package to public.

if you wanna test it, this minimum code to make it work:

function App() {
  return (
    <ARNFTCanvas>
      <ARNFTMarker markers={['/markers/pinball']} persisted>
        <Box args={[100, 100, 100]} />
      </ARNFTMarker>
      <ARNFTMarker markers={['/markers/insta']}>
        <Sphere args={[100, 100, 100]} />
      </ARNFTMarker>
    </ARNFTCanvas>
  );
}

p.s. there gonna be a bit of refactor/cleanup when i manage to make worker portable

antokhio commented 1 year ago

@kalwalt the problem is line 706 https://github.com/webarkit/jsartoolkitNFT/blob/9277579a055a20ec1bee1e8f9437b273ea9806ed/src/ARControllerNFT.ts#L706 this prevents call with correct signature.... likely it's just misstyped

however according to this: https://github.com/webarkit/jsartoolkitNFT/blob/9277579a055a20ec1bee1e8f9437b273ea9806ed/src/ARControllerNFT.ts#L100-L105

the return type should be:

 {id: number}[]

not sure, looks like an error in both places actually according to my code it returns:

 {
    id: number;
    width: number;
    height: number;
    dpi: number;
}[]
kalwalt commented 1 year ago

@antokhio i will try to fix in a PR i hope soon.

kalwalt commented 1 year ago

with the latest in PR #231 it should works in this way:

await ar?.loadNFTMarkers(nftMarkerUrls, (id: number[]) => {
      let marker = ar.getNFTData(id[i], 0);
      ctx.postMessage({ type: "markerInfos", marker: marker });
      ar.trackNFTMarkerId(id[i]);
      console.log("loadNFTMarker -> ", id);
      console.log(id);
      ctx.postMessage({ type: "endLoading", end: true });
      }, () => {}).catch((err: string) => {
          console.error("Error in loading marker on Worker", err);
      });