Twinklebear / tobj

Tiny OBJ Loader in Rust
MIT License
233 stars 47 forks source link

Multi index #40

Closed virtualritz closed 3 years ago

virtualritz commented 3 years ago

As discussed here.

virtualritz commented 3 years ago

I closed this because I have many more changes that I would like you to have a look. I merged all three branches with those into my master branch. That said, everything can easily be cherry-picked.

Twinklebear commented 3 years ago

Cool no worries, yeah I have to make some time to check it out there's quite a few things added. For ease of migration, can we keep the single index mode as the default instead of having to set single_index: true? For folks using the library already this will make it easiest to update their code to the new API by just using the then default load options.

virtualritz commented 3 years ago

Can we keep the single index mode as the default instead of having to set single_index: true? For folks using the library already this will make it easiest to update their code to the new API by just using the then default load options.

This is only half true. Once you added the triangulate flag everyone had to touch their code with their brain switched on because that was not an Option<bool> but a bool. So now people who had set this to true have to write:

LoadOptions {
    triangulate: true,
    single_index: true,
    ..Default::default(),
}

The reason I set single_index as false by default is for LoadOptions::default() to mean: give me the contents of the OBJ file, with as little as possible/no modification at all.

From the LoadOptions Default trait docs:

Options for processing the mesh during loading.

By default, all of these are false. With those settings, the data you get represents the original data in the input file/buffer as closely as possible.

If single_index is true this promise is broken. Isn't this nice property of the defaults maybe worth the small hassle of writing four lines, instead of three, to replace what was a single bool before?

Also note that LoadOptions is passed in by reference.

The idea was that a user may want to declare a const MY_LOAD_OPTIONS somewhere and then use that throughout the code instead of repeating the init struct pattern every time. Assuming they even have more than one call to load_obj()/load_obj_buf() in their code.

Twinklebear commented 3 years ago

Yeah I like the pass by ref, then folks can just have some global const thing as you mention.

Ok let's keep it as all false then, the default of just giving the OBJ file content directly makes sense, and the docs are clear about it.