Implementing CommonMCOBJ Support

[StandingPadAnimations]

This implements parsing and handling for CommonMCOBJ headers in OBJs, and reworks exporter checking code to be more exporter agnostic.

CommonMCOBJ is a new exporter standard that Mineways and jmc2OBJ are currently working on supporting. CommonMCOBJ provides more information for MCprep to utilize for OBJs, and is beneficial overall in the long run.

When an OBJ with a CommonMCOBJ header is imported, MCprep will parse the header and add all parsed options as custom properties to the OBJ. In addition, as the plan is to phase out global exporter options in MCprep 4.0, OBJs without the CommonMCOBJ header will gain an additional MCPREP_OBJ_EXPORTER property, for backwards compatibility in MCprep 4.0. OBJs without either the CommonMCOBJ properties or the MCPREP_OBJ_EXPORTER will fall back to the globally set exporter (though globally set exporters will become a feature behind a legacy flag in MCprep 4.0)

When it comes to exporters, CommonMCOBJ is exporter agnostic, and provides a key for the exporter name. To handle that, all exporter checks in MCprep will now be done with an enum rather than pure strings. The options in the enum are:

• Supported exporters with CommonMCOBJ (Mineways and Jmc2OBJ)
• Cmc2OBJ (which for the most part is treated as unsupported, but exists as a distinct option for testing purposes)
• ClassicMW (Mineways before CommonMCOBJ) and ClassicJmc (jmc2OBJ before CommonMCOBJ), which uses either the globally set exporter in the N panel or the MCPREP_OBJ_EXPORTER attribute on the active object
• Fallback Unknown for all other exporters

EDIT 30/03/2024: commonmcobj_parser.py is, unlike the rest of MCprep, licensed under the BSD 3-Clause license to allow developers to reuse without worrying about GPL. As such, the README has been updated to account for that, and LICENSE-3RD-PARTY.txt has also been added.

Closes #553

[TheDuckCow]

Great to have this support! Small but important request from my side: can you implement at least one parsing test? I suggest you do so by adding manually created, simple obj file with and without a header which is saved into the testing directories of the addon (so it's not shipped). This is the number 1 way we can ensure it continues to function as intended and to spec, though I'm ok if the test isn't exactly exhaustive over all scenarios.

Let me know if you have trouble running the tests, they should be nicely cross platform by now though.

[StandingPadAnimations]

@TheDuckCow by the way, should we implement the "legacy" flag for OBJ imports now and have it enabled by default (then disable it later on) or should we wait until MCprep 4.0 (which if I recall correctly will have UI refactorimg anyway)

[StandingPadAnimations]

Alright, normally I'm not a fan of dual licensing, but I've decided to put the CommonMCOBJ parser under the more permissive BSD 3-Clause license (compatible with GPL), mainly to make it easier for developers to reuse in their own projects without worrying about GPL. This should be fine as the parser doesn't interact with BPY and BSD 3-Clause is compatible with GPL.

If wanted, I'm fine with changing it back to GPL, but I'd rather have it under a more permissive license (especially as it doesn't depend on Blender and could be reused anywhere)

EDIT: only the CommonMCOBJ parser is under a separate license (as it's really an independent component), the rest of MCprep is still under GPL

[TheDuckCow]

Yup I'm supportive of this, thanks for thinking of it. Will review when I'm back home later On Mar 30, 2024 12:48 PM, Mahid Sheikh @.***> wrote: Alright, normally I'm not a fan of dual licensing, but I've decided to put the CommonMCOBJ parser under the more permissive BSD 3-Clause license (compatible with GPL), mainly to make it easier for developers to reuse in their own projects without worrying about GPL. This should be fine as the parser doesn't interact with BPY and BSD 3-Clause is compatible with GPL. If wanted, I'm fine with changing it back to GPL, but I'd rather have it under a more permissive license (especially as it doesn't depend on Blender and could be reused anywhere)

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

[StandingPadAnimations]

Alright, I've added LICENSE-3RD-PARTY.txt (not the best name as commonmcobj_parser.py isn't 3rd party, but oh well) to indicate the license for commonmcobj_parser.py and added clarification in the README regarding it.

[StandingPadAnimations]

I've added some basic forward compatibility by adding the original header to the object

[StandingPadAnimations]

• let's get the tracker assignments updates so we still get the exporter names (ideally with a _cmcobj suffix or similar, so we can di sql grouping/breakdowns later). For legacy imports, we should still get the legacy names as we have before, so we can compare equally.

Alright, sounds good

• Having a single source which has the responsibility of what represents the "world", so that all the raw data isn't duplicated everywhere. I think that would mean having the data stored in the collection that gets generated, or optionally in a top level parent (empty?) object. If someone moves things around outside the collection, they probably aren't expecting it to stay connected anymore anyways, as that's sort of like breaking the link.

Are custom properties for collections supported in older versions of Blender? I'm fine with moving from adding properties to all objects to just collections, but that's my main concern.

• At least one unit test covering the commoncobj _parser.py file. We have increasingly decent test code coverage with MCprep. I think it would be great if we added at least one that tried to make some coverage of the common obj implementation. I'd be ok even if it was a super simple one, one that just calls the main functions, passing in onw or two known scenarios, and checking we get the known output assignments in the class too.
• A step further would also be adding a test for the world importer operator, with known world exports (tiny in size, like literally just a single block or two) from Mineways + jmc2obj in the new format, and checking those in test objs to run against and verify we get the correct params set (not checking in textures since they shouldn't be necessary to the test code).

cmc2OBJ is the reference implementation of CommonMCOBJ, so ideally we should use that for CommonMCOBJ tests (it has some known issues though with material exports, but it would only be used for CommonMCOBJ tests anyway)

[TheDuckCow]

Are custom properties for collections supported in older versions of Blender? I'm fine with moving from adding properties to all objects to just collections, but that's my main concern.

I'm not sure, would be worth checking into. I'm thinking a couple steps ahead to the import bridge, where different chunks of the world would be loaded together (probably larger than mc chunks though). I'd expect they would all live within one "world" collection, but each individual chunk should have its own "door". That could be a sub collection or event an empty parent. What do you think, would it be very annoying if all world imports had a collection and also a root empty parent? When that root is selected we show special drawing features for instance potentially. Although the same could be also accomplished through looking at the active objects parent collection/object too.

cmc2OBJ is the reference implementation of CommonMCOBJ, so ideally we should use that for CommonMCOBJ tests (it has some known issues though with material exports, but it would only be used for CommonMCOBJ tests anyway)

Totally fine and onboard with having the core tests stay over there, that's fair. I'd like some kind of integration test in MCprep just to make sure that everything is glued together ok. Could be an extension of a world import operator unit test, hence the idea of having a small checked in obj with the spec.

Thanks for working on these!

[StandingPadAnimations]

I'm not sure, would be worth checking into. I'm thinking a couple steps ahead to the import bridge, where different chunks of the world would be loaded together (probably larger than mc chunks though). I'd expect they would all live within one "world" collection, but each individual chunk should have its own "door". That could be a sub collection or event an empty parent. What do you think, would it be very annoying if all world imports had a collection and also a root empty parent? When that root is selected we show special drawing features for instance potentially. Although the same could be also accomplished through looking at the active objects parent collection/object too.

I think an empty acting as the parent for chunks is a good idea, it also makes moving OBJs easier for users.

One issue I realized last night about collection properties it that if the user has a non-OBJ object in said collection (like a plane or rig), it would be interpreted as part of the OBJ, so an empty is a better idea IMO.

Totally fine and onboard with having the core tests stay over there, that's fair. I'd like some kind of integration test in MCprep just to make sure that everything is glued together ok. Could be an extension of a world import operator unit test, hence the idea of having a small checked in obj with the spec.

I'm fine with integration tests in MCprep for CommnMCOBJ, just that we should use OBJs exported from cmc2OBJ for tests as cmc2OBJ is the reference implementation of CommonMCOBJ, so ideally exporters should replicate their common header output to match cmc2OBJ.

Btw, on the topic of the tracker, is there any extra work that needs to be done tracker side to support new exporter strings, or is it fine as is?

[TheDuckCow]

Hey @StandingPadAnimations getting back to this. One question, since we're nearing to a release. Do we want to merge the milestone 360 branch into dev before or after this common mc obj branch? I'm in favor of merging sooner so we can get everything in order, and avoid us having to replicate changes across multiple branches.

As for the changes above, I'd say: let's go ahead with empty parents for chunks.

integration tests in MCprep for CommnMCOBJ, just that we should use OBJs exported from cmc2OBJ for tests as cmc2OBJ is the reference implementation of CommonMCOBJ, so ideally exporters should replicate their common header output to match cmc2OBJ.

Works with me 👍

Btw, on the topic of the tracker, is there any extra work that needs to be done tracker side to support new exporter strings, or is it fine as is?

Nope, it accepts anything you feed into it. Which is why I'm picky in the the review stage as it'd be a pain to override later :)

[TheDuckCow]

Just remember to re-request my review once it's at a stage for me to hands on with the empty approach.

[StandingPadAnimations]

Hey @StandingPadAnimations getting back to this. One question, since we're nearing to a release. Do we want to merge the milestone 360 branch into dev before or after this common mc obj branch? I'm in favor of merging sooner so we can get everything in order, and avoid us having to replicate changes across multiple branches.

As for the changes above, I'd say: let's go ahead with empty parents for chunks.

integration tests in MCprep for CommnMCOBJ, just that we should use OBJs exported from cmc2OBJ for tests as cmc2OBJ is the reference implementation of CommonMCOBJ, so ideally exporters should replicate their common header output to match cmc2OBJ.

Works with me 👍

Btw, on the topic of the tracker, is there any extra work that needs to be done tracker side to support new exporter strings, or is it fine as is?

Nope, it accepts anything you feed into it. Which is why I'm picky in the the review stage as it'd be a pain to override later :)

I'm fine with merging milestone-3-6-0 before this one. I'll just change the base branch to dev and we can go ahead with the merge. An approximate ETA for this would be the end of next week, so merging milestone to dev wouldn't be a biggie.

With the merge to dev, I think we should get at least 1 or 2 Release Candidates (RCs) before release, considering MCprep 3.5+ ignores RCs in the updater now (allowing us to make public releases). MCprep 3.6 includes a lot of major changes, and we haven't really tested in production yet. If we merge today or tomorrow, I can get a RCs made tomorrow or Monday (for context, RCs for MCprep 3.5 were made on Sundays).

[StandingPadAnimations]

Alright, changed the base to dev, we can go ahead with merging milestone-3-6-0

[StandingPadAnimations]

In an effort to try and implement the empty at OBJ center feature, I've come across an issue with how OBJ's are handled in Blender,

export_bounds_* should correspond to the OBJ geometry, and they do, but only as a local transform (when going into edit more on an OBJ and checking the transforms). However, when creating the empty, the empty's coordinates are in world space, which is completely different. Not sure how I can resolve this

[TheDuckCow]

Hey @StandingPadAnimations if I'm understanding the problem correctly, maybe we can solve it by adding an extra property/meta data field on each chunk obj which is the chunk offset or index or something like that?

That should give us the information to do the practical translation from one coordinate system to the other. What do you think?

[StandingPadAnimations]

I would rather avoid adding additional keys to the spec, mostly because V1 hasn't even been fully adopted yet (and V2 isn't due till 2025 anyway). Plus, I think this is more of a Blender issue then an OBJ one.

The thing is, export bounds are correct in the exported OBJs, but only if you go in edit mode and look at local transforms (global transforms are where the issue lies). If we could somehow set the location of an empty based on those local transforms, it'd work fine as far as I know.

[TheDuckCow]

I wouldn't think of this as part of the spec, but something specific to MCprep which is an internal nuance of how it handled the spec. So in terms of having a pure python presentation, this property would not live there. This would not be anything any other implementation of the spec outside MCprep would even make sense for, just some intermediate caching. The property could even potentially be save based on naming the chunk empty itself, so "1_1"or "0_-5" couldnbecome.the empty name, and by parsing that we could know the offset.

The other thought could be: do we even need to worry about this offset? If we parent the meshes with initial transforms to the empty location, we can always negate it by checking the transform of any one single object to get back at the offset. So maybe we don't need to save any extra data at all? (That would break if anyone applied all transforms to make it global, whereas asking people to not rename the names of Empty chunks is probably a little bit more reasonable and understandable)

[StandingPadAnimations]

Sounds good. To clarify, what exactly would chunk offset refer to?

[TheDuckCow]

Sorry missed this comment. It's whatever you need to have it refer to in order to get around the change from one coordinate reference point to the other :) So whatever you choose that makes sense, I trust your judgement on that. Like I said in my mind, could either be chunk counts or even direct block unit offsets. Direct offsets are probably safer, in case we eventually implement set extensions which allow for variable size chunks or something like that

[StandingPadAnimations]

So by direct offsets, you mean parenting the OBJ to the generated empty at say (0,0,0) so we could determine the actual offset if needed? That could work, but the origin of OBJs is also at (0,0,0), so the offset would always return 0.

[TheDuckCow]

@StandingPadAnimations

So by direct offsets, you mean parenting the OBJ to the generated empty at say (0,0,0) so we could determine the actual offset if needed? That could work, but the origin of OBJs is also at (0,0,0), so the offset would always return 0.

Fair to make sure we're totally explicit here. So, I'm imagining something akin to this file setup:

Does this solve the challenge you were facing in the end? In the end, it's just whatever value is needed to get back at the spec compared to whatever transform setup we establish. The offset could just mean whatever translation amount was needed to be applied to the children when parenting to the empty for instance. I did a manual object property here, but presuming that we'd have the rest of the commobobj format also saved to this empty object. For typical world imports, it'll be a single root empty. For cases where we support batch loading of chunks (hmm similar to what MiEx does I guess), then there will be multiple empties for each chunk. The user could even choose to move this chunks to other collections (think, disabling far collections as you focus on just the near).

[StandingPadAnimations]

Makes a lot more sense now, thanks! I've also checked to see how coordinates match in Blender, and it seems like if we inverse the bound's Z coordinate, the empty would be in the correct location. At least, that's how jmc2OBJ/cmc2OBJ behave, I haven't tested with Mineways yet. So in essence, given a bound (X, Y, Z), the empty would be at (X, -Z, Y) (Z swapped with Y since Y is up in Minecraft)

[StandingPadAnimations]

(If Mineways doesn't show the same behavior, that might be something we could look into clarifying in V2, but I assume Mineways does)

[StandingPadAnimations]

Alright, Mineways also shows the same behavior (although it should be noted both the initial Z and the Z offset need to be negated, but that's a given), so we're good. I'll still add a specification in V2 just in case.

[TheDuckCow]

Fantastic! Will happily await once you're ready to re-request me so I can try out with a couple worlds. Thanks a ton again for driving this forward!

[StandingPadAnimations]

Finally got this in (though it took a bit of pain as Blender's matrix transforms are weird)

[StandingPadAnimations]

Also I just realized this PR is reported to be larger than the actual changes, probably due to that accidental squash merge. We should probably revert that squashed commit, then remerge the milestone branch properly

EDIT: Created #571 to address this issue

[StandingPadAnimations]

Updated this branch with the remerge in dev. Sadly, we still have some extra commits, but it should be more manageable now

EDIT: correction, I just remembered that we merged #549 into this branch so we could merge the milestone branch, so these aren't actually duplicate commits

[StandingPadAnimations]

@TheDuckCow we're good to go for review. Just a forewarning that there's bit more stuff to review now as #549 was merged into this branch 2 weeks ago.

[TheDuckCow]

Hey @StandingPadAnimations did a very quick test and ran into this issue below while importing an old world I have.

Traceback (most recent call last):
  File "/Users/patrickcrawford/Library/Application Support/Blender/4.1/scripts/addons/MCprep_addon/tracking.py", line 887, in wrapper
    res = function(self, context)
          ^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/patrickcrawford/Library/Application Support/Blender/4.1/scripts/addons/MCprep_addon/world_tools.py", line 772, in execute
    self.track_exporter = addon_prefs.MCprep_exporter_type  # Soft detect.
                          ^^^^^^^^^^^
NameError: name 'addon_prefs' is not defined

Then, tried the test runner. It seems we have 19 tests failing, so would try running through this again when you have a chance. Maybe it's an issue with handling OBJs that don't have any spec data?

(3.6.2)   	all_tests	65	2	19	test_prep_material_animated;test_prep_missing_pbr_passes;test_prep_pbr_seus;test_prep_pbr_specular;test_prep_simple;test_replace_missing_images_animated;test_replace_missing_images_fixed;test_replace_missing_images_moved_blend;test_replace_missing_images_name_incremented;test_swap_texture_pack [jmc2obj_noprep];test_swap_texture_pack [jmc2obj_withprep];test_swap_texture_pack [Mineways_withprep];test_meshswap_world_jmc;test_meshswap_world_mineways_combined;test_meshswap_world_mineways_separated;test_world_import_jmc_full;test_world_import_mineways_combined;test_world_import_mineways_separated;test_convert_mtl_simple
tests took 13s to run, ending with code 1