wizardgsz commented 5 years ago

Please review my contrib, if you have time obviously :D Kind regards

wizardgsz commented 5 years ago

Instead of .gitignore, I'm using /git/info/exclude now

RichysHub commented 5 years ago

I'm going to go over this a couple more times, just to check I'm happy with everything. Also going to manually test the imports (because this repository is lacking proper testing).

wizardgsz commented 5 years ago

Maybe you already have enough "test cases", I downloaded sample data from:

https://github.com/mikelovesrobots/mmmm https://github.com/ephtracy/voxel-model https://github.com/marcosecchi/naba-2016-workshop-assets https://github.com/kluchek/vox-models

Btw, I found this interesting parser also (unfortunately it does not handle new chunks): MagicaVoxel File format spec for Kaitai Struct

RichysHub commented 5 years ago

Oh, nice find on that parser (though it's not a system I'm familiar with)

I developed this based on ephtracy's original spec, and have had a rewrite on the horizon to the new spec.

RichysHub commented 5 years ago

I've not used Blender 2.80 at all, but this feels less than ideal as an end result for the scene collection.

This (manually edited) result feels more "correct" to me? Not sure how feasible this would be within the current framework.

wizardgsz commented 5 years ago

I think we could add every voxel in a new "collection" (the collection is a new structure in 2.80), exactly as you planned in the above screenshot (and remove the "base" cloned cube). Official SVG importer, for example, works this way. You are right.

Something like this:

create new collection

name = os.path.basename(path)
collection = bpy.data.collections.new(name)
bpy.context.scene.collection.children.link(collection)

...and modify the later "link" to scene collection

for object_ in to_link:
    collection.objects.link(object_)

wizardgsz commented 5 years ago

Moreover, I'd like to "collapse" the newly created collection, but it seems there is no API for it yet :(

There is a workaround but I do not like to use workarounds usually: https://devtalk.blender.org/t/view-layer-api-access-wishlist-collection-expand-set/5517/3 https://blenderartists.org/t/question-regarding-expanding-collapsing-collection-in-outliner-in-2-8/1175242

wizardgsz commented 5 years ago

Should we delete "base_voxel" at the end?

def delete_object(obj):
    # Deselect all
    bpy.ops.object.select_all(action='DESELECT')
    obj.select_set(True)
    # Call the operator only once
    bpy.ops.object.delete()

Instead of (de)selecting the object(s) beforehand, you can also pass a custom context (a list of objects) to avoid changing the selection within the viewport:

# Delete the template cube
bpy.ops.object.delete({"selected_objects": [base_voxel]})

wizardgsz commented 5 years ago

About "default palette": please double check the unpack big/little endian mode, I think it should be corrected in

struct.unpack('<4B', struct.pack('<I', DEFAULT_PALETTE[col]))

I verified it loading this VOX without custom paletter i.e. RGBA chunk: char_cat.vox

    if not palette:  # no palette provided, use default
        for col in range(256):
            palette.update({col + 1: struct.unpack('<4B', struct.pack('<I', DEFAULT_PALETTE[col]))})

RichysHub commented 5 years ago

Okay, have taken some time to look over the changes.

Very happy with the collection organisation, definitely an improvement. 👍
I agree on not using a workaround for something that's non-critical (collapsing group) ✔
Deleting of the initial cube, though annoying, might be the simplest way to end clean 🆗
Current implementation doesn't leave a direct copy of the initial voxel (the one we clone). Probably best to just not peel the first voxel data at all, and create the cube at origin. ❗❗

With regards to color, this came up in issue #5, and was inverted in commit 9e79292. I would suggest some thorough investigation here, because I am unsure. ❔
Small research into point above made me realise material.diffuse_color = [pow(col / 255, gamma_value) for col in palette_entry[:4]] is gamma correcting the alpha channel, which I believe is incorrect. ❗

wizardgsz commented 5 years ago

Any other open point? Next, I'd like to work on material properties in MATT chunk...

wizardgsz commented 5 years ago

Removing the special first voxel, making it instead a generic cube at 0,0,0. This fixes issue with missing voxel in end result

I think we have to modify the "bounds" properties start_voxel and end_voxel. Are they 1-based in your implementation?

RichysHub commented 5 years ago

    if use_bounds:
        # clamp end_voxel to size of model
        end = min([end_voxel, len(voxels)])
        voxels = voxels[start_voxel:end]

Voxel bounds are definitely implemented as zero based, but then the slider in the UI defaults to 1, and is min of 1.

Well spotted! 👍

Bounds were added to have a chance of loading big models, so weren't implemented with much care. I have no strong feelings either way, but yes, we do need to unify the two. Also a label or something in the UI might be good to inform.

RichysHub commented 4 years ago

Okay, I don't think there's anything outstanding that needs doing.

I'll wait until I have your okay to do so, but I think this is ready to be squashed together and merged in. 🎉

wizardgsz commented 4 years ago

Nice, thanks! I think we are ready to close the pull. I'll open a new request for MATT/MATL and new chunks if you agree.

RichysHub commented 4 years ago

Hmm, I wonder if there's something smarter we could do here. There's a lot of commits in this pull (25) which would make it more than 2/3 of the total repo commits. I don't want to just merge them all in, and leave a messy git history. So I was about to squash this whole thing down into a single commit, and merge it in.

However, that leaves anyone using 2.74 having to use the older version. That's fine, and I intended just to tag the relevant point, maybe make it a release on GitHub to make it super clear for anyone still working <2.80.

But, in this pull request you've also identified and fixed a couple bugs (voxel bounds, MATT chunk skipping, and palette). Ideally those would be fixed in the 2.74 leave-off version.

I don't know how much control you have with regards to rearranging commits, and I'm not a git guru myself.

If we could get this pull down to just:

fixing bugs in 2.74 (either 1 commit, or a couple)
porting to 2.8

Then we could merge all the commits separate, have a 2.74 point with fixed bugs, and have a clean history.

I'm going to do some reading, see if I can cherry pick out the bugfix commits and patch them on first.

wizardgsz commented 4 years ago

There is an option to check for Blender version using bpy.app.version:

if (2, 74, 0) < bpy.app.version:
    print("Here it is the old code")

But I think the code will be less "intuitive" to read and maybe harder to maintain

I agree: you can start "fixing" the few minor bugs (the palette should be ok in your old implementation for 2.74, I added a bug gamma-correcting apha values instead).

I am not a "Git" expert unfortunately: it is the first time I use it (I am used to the old SourceSafe or later TFS by Microsoft).

RichysHub commented 4 years ago

Alright, I'll get reading.

Once we start adding new features to the 2.80 importer, I'll put some brain power towards how we could have a single script work for both, without being a huge ugly mess.

RichysHub commented 4 years ago

Okay, I managed to move things around to get these changes merged into master. Separated out your bugfixes, so hopefully Blender <2.80 can enjoy them. Have tagged 2 releases, for the relevant Blender versions.

I have had to rebase some things, so if you have built upon the code in this pull request, you will have to rebase onto the master branch. I can help you with that in the relevant issue.

Thank you for your hard work on getting this pull through 🎉🎊

RichysHub / MagicaVoxel-VOX-importer

Ported to 2.80 syntax #8

create new collection