search

CesiumGS / obj2gltf

Convert OBJ assets to glTF
Apache License 2.0
1.71k stars 307 forks source link

Don't save out two bins #62

Closed lilleyse closed 7 years ago

lilleyse commented 7 years ago

Thanks for the report @emackey

As of now, the bins are guaranteed to have different names, so removing the first won't be an issue.

emackey commented 7 years ago

Is there any way this could remove the wrong file, or any way that the two bin filenames could both end up as the same filename?

lilleyse commented 7 years ago

gltf-pipeline currently saves out the bin as the id of the buffer, which will usually be called buffer. obj2gltf at least names the id buffer before sending it across.

Meanwhile the intermediary step in obj2gltf saves it as [filename].bin. The one situation that would fail is if the model is buffer.obj.

I was contemplating having gltf-pipeline write [filename].bin but there was too much extra work to make that happen, this seemed like the easier fix.

mramato commented 7 years ago

The correct solution is that any temporary/intermediate files should be written as random temp filenames in the operating system's temp location. We should not use the output directory as a temporary output location.

lilleyse commented 7 years ago

Updated to use a temp directory instead.

lilleyse commented 7 years ago

This is ready for further review/testing. Would like to get this in with the next publish. @emackey @mramato

mramato commented 7 years ago

This all looks good to me. I don't think either me or @emackey have commit access her.

@likangning93 can you do a quick review and merge?

lilleyse commented 7 years ago

I did some test cleanup inspired by https://github.com/AnalyticalGraphicsInc/obj2gltf/pull/62#discussion_r113226732. @likangning93 ready to review now.

lilleyse commented 7 years ago

@likangning93 no need to review, but can you merge this?

mramato commented 7 years ago

@emackey you have commit access now, feel free to merge.

emackey commented 7 years ago

Thanks!