IFCLoader parse should return Mesh

[xawill]

• three version: 0.129
• node version: 12.16.0
• npm version: 6.13.4

Problem description:

I think the parse method of the IFCLoader should return something and not void, isn't it @agviegas? For instance with the ColladaLoader, the user can directly use the parse method with the content of the Collada file as a string or a Buffer.

I saw you recently updated the typings for your library, but this remained untouched.

From what I read in the code, I think it outputs a Mesh (i.e. the output of the function generateAllGeometriesByMaterial).

Relevant code:

parse(data: ArrayBuffer | string): void;

Suggested solution:

parse(data: ArrayBuffer | string): Mesh;

[xawill]

Or maybe to maintain consistency with other loaders it should return IFC ?

[agviegas]

Sure, we can easily do this! 😅 Sorry, we have so many things to consider that sometimes we miss this kind of things.

[xawill]

No worries @agviegas. Actually, parse is async, so it would be Promise<IFC> and parse should be marked as async in the typings as well.

Thank you very much !

[xawill]

How do we proceed @agviegas? I can create the PR if you don't have much time, but I don't have a fork of this repository (yet). So maybe it is more efficient if you do it, especially if you plan other changes in the typings in the short term. What do you think ? Again, I can definitely do it if you can't.

[agviegas]

I can do it tomorrow!

[agviegas]

Hey, I'm checking it right now 🤔 We are currently working with the onload method to get the result of the Loader. Isn't this enough? This is the current implementation. What I really don't know is what difference should we make to be able to define IFC (an extension of Object3D) the return type.

[xawill]

The use case to use the parse function directly is if you want to allow the user to load his own file, i.e. if the file doesn't have a url and you only have the ArrayBuffer (like the three.js editor does).

To recap, here are the things to consider for the parse function:

• Marking it as async and returning a Promise<>: I think we both agree on that, as you do onLoad(await scope.parse(buffer));.
• Defining the buffer type to ArrayBuffer: it could technically be string | ArrayBuffer, but I noticed the IFCLoader doesn't work if we feed it the content of the file as a string.
• Returning an IFC: It could technically be an Object3D, but as said here above, it should be IFC to maintain consistency with other loaders which use their own type. And you do too: onLoad: (ifc: IFC) => void. You rightfully indicate that in the onLoad callback, you will give back an IFC (which is the output of await scope.parse(buffer)).

I don't know why you defined an IFC type in the first place (I don't see it much used in the code), but this is a good practice. If an IFC is exactly an Object3D, you can either get rid of IFC or force-cast the Object3D to IFC (with as IFC). Anyway, you should make sure that this.ifcManager.parse(buffer) returns IFC.

So IMO it should be

async parse(buffer: ArrayBuffer): Promise<IFC> {
  return this.ifcManager.parse(buffer);
}

Also, I didn't really understood how you defined the IFC type. But if this is exactly an Object3D, I think it will reduce your headache if you define it like this: type IFC = Object3D, no?

And I noticed that the url parameter in the load function is marked as any. Is it on purpose ? Shouldn't it be string?

[agviegas]

Everything clear, I'll go on with those changes! 😄 Thanks for the throughout explanation!

[agviegas]

Sorry, I was busy before, but I'm refactoring the types now. This is what I'm doing, which I think is in line with this. I'll also implement your ideas!

[xawill]

Yep, mrdoob suggestions are definitely in line with mine. They are just more up to date with the current version of the IFCLoader. Good job!

[xawill]

What about this @agviegas ? It looks like you solved it in PR #104, but I was surprised to see that it wasn't merged for r131.

[joshuaellis]

What about this @agviegas ? It looks like you solved it in PR #104, but I was surprised to see that it wasn't merged for r131.

The PR has lint errors that need to be resolved before we can merge.

[agviegas]

@xawill sorry, there was another change request and I forgot to resubmit both here and in three.js. I'll try to do it now.

[agviegas]

FYI, there are some issues due to some changes of Three.js. Tomorrow I'll go on vacation and won't have access to my computer until 17th/ from that day this will be my first TODO.