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.

[TheDuckCow]

Just leaving some initial comments

[TheDuckCow]

I think the tracker is still worthwhile, as it helps us understand the ecosystem of what's being used and how to best support everyone.

Main question is: do we have the header auto assign a value of e.g. jmc2obj to the exporter type, or should we add two more types which are jmc2obj_cmobj (or something like that) and the same for Mineways? I'm a bit torn. It could be quite nice to know how well adopted the common format is, to know what feature work and whether it applies to a good % of the MCprep userbase.

[StandingPadAnimations]

I think we could use the output of get_exporter (something like get_exporter(context).value to retrive the underlying string from the enum, assuming it's not None) as it would be a 1-1 mapping, although we'd then have to concert the Mineways and jmc2OBJ references in the tracker to mineways-c (Mineways before CommonMCOBJ) and jmc2obj-c (jmc2OBJ before CommonMCOBJ). It would also allow tracking the use of unknown exporters as well

[StandingPadAnimations]

Thinking abut this further, I think we should use the string in the exporter key of the header. If we use the enum, we won't be able to know if a new OBJ exporter is gaining traction (and thus could be better supported in MCprep), but the exporter key would make it more clear what specific exporters are being used

[TheDuckCow]

Remind me, what would be the values we potentially get in the exporter? that's the mineways-defined or jmc2obj-defined names for themselves, right?

I think having something like jmc2obj_cmobj would be fantastic or at least somehow_ knowing which imports comes form the commonmcobj spec in the analytics data. A canned suffix added to the exporter string could do. Knowing the depth of adoption of the new standard helps understand how many people can make use of a tool we build that depends on that (if e.g. less than 10% of obj imports are using the spec, that could be good rationale to not yet spend ages working on the bridge tool just yet for instance).

[StandingPadAnimations]

Remind me, what would be the values we potentially get in the exporter? that's the mineways-defined or jmc2obj-defined names for themselves, right?

It would be the name of the exporter, so jmc2obj, mineways, helpicantexitvim, etc. For the pre-CommonMCOBJ exporters, we could put jmc2obj-classic and mineways-classic, and convert our existing data (a cmobj suffix works, but I'm not the biggest fan of the idea)

[TheDuckCow]

Also based on the implementation above, wouldn't the readlines() line within parse_header clear out the remainder of readlines below? Would be good to double check all tests still pass as you're developing here.

[TheDuckCow]

Turns out this seems to be the source of the problem, indeed the parse_header above if commented out makes more of the tests pass. Which also means we really do need to add also tests for the common obj part too, otherwise we're not really ensuring this continues to work.

[TheDuckCow]

Through investigating this, I have also come to realize the existing commonmcobj parser actually always parses the entire obj file, line by line, in search of the header. This could be a massive slowdown for large obj files.

FYI I'm looking into the best option to short circuit @StandingPadAnimations, most likely if we haven't seen the anchor COMMON_MC_OBJ_START within the first 100 lines of the header, then we're probably safe to say it's safe to exit early. Started this diff locally, will need to verify against actual Mineways and jmc2obj example commonmcOBJ exports when I get a chance:

    # Read in the header
-   for l in f:
-       tl = " ".join(l.rstrip().split())
-       if tl == "# COMMON_MC_OBJ_START":
+   lines_read = 0
+   for _l in f:
+       tl = " ".join(_l.rstrip().split())
+       lines_read += 1
+       if lines_read > 100 and tl and not tl.startswith("#"):
+           break  # no need to parse further than the true header area
        elif tl == "# COMMON_MC_OBJ_START":
            header.append(tl)
            found_header = True 
            continue

[StandingPadAnimations]

CommonMCOBJ allows for headers to be anywhere in the file technically, either in the beginning or the end. That said, we could check for both 100 lines in the beginning and 100 lines at the end, and allow it to be configurable for users

[TheDuckCow]

I briefly looked at efficiently reading the last N lines of a file without actually iterating over all of them. It's a little complicated. The other more practical consideration is that both jmc2obj and Mineways, as I've just verified, put commonobj towards the top. Currently in both exports I've done, the header starts ~line 55, and for jmc2obj it's at the very start. So, Seems like a buffer of the first 100 should be safe. I might time-test the difference later to see how much it would end up saving

[StandingPadAnimations]

If we do implement a 100 line limit, then we should also allow the user to be able to configure it somewhere. That way, if an OBJ that is a CommonMCOBJ export doesn't put it in the first 100 lines, users wouldn't have to go down into the Python code and modify the limit.

I think a good way to test would be to have an OBJ file where the header is at the end (which should be trivial since OBJ is a text format) and benchmark the time difference in MCprep. Not only would it give us insights into whether or not it makes a practical difference, it would also test to see if any unexpected issues occur

[TheDuckCow]

I'd be in favor of marking that a TODO if you're ok way it, to keep parsing simple and fast for now (otherwise it'll be seen as a regression).

Also spoiler I will not finish this tonight, I had to update and create a test world for v1.21 of MC and do exports for both jmc2obj and mineways. While that is working, it perhaps has revealed some issues with the mapping script which I am looking into still along with setting up tests to make sure parsing is working against real commonmcobj world exports.

[StandingPadAnimations]

Sounds good to me

[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]

Ran the tests locally on my machine (after updating this branch with dev, I'll push that merge after I finish some other things):, got the same result as you did (as well as GitHub Actions):

bversion        ran_tests       ran     skips   failed  errors
-------------------------------------------------------------------------------
(4.1.1)         all_tests       68      2       3       test_swap_texture_pack;test_convert_mtl_simple;test_world_import_cmcobj_jmc_full [test_mappings]
tests took 41s to run, ending with code 1

This confirms that now, tests are reproducible (at least on macOS, GitHub Actions with the Ubuntu runner, and Arch Linux on bare metal)

[StandingPadAnimations]

One of the errors I'm getting locally is the following:

Error: File not found, could not import obj '/home/mahid/Documents/MCprep/test_files/fake_world.obj'

Are we missing some files in git-lfs?

EDIT: just realized this might be intentional

[StandingPadAnimations]

Alright resolved test_convert_mtl_simple; this was due to changes in the return values for convert_mtl, where None now represents a success and MCprepError represents a failure, with booleans being removed.

With some fixes to the test, my results look as such:

-------------------------------------------------------------------------------
bversion        ran_tests       ran     skips   failed  errors
-------------------------------------------------------------------------------
(4.1.1)         all_tests       68      2       2       test_swap_texture_pack;test_world_import_cmcobj_jmc_full [test_mappings]
tests took 38s to run, ending with code 1

[StandingPadAnimations]

Got test_swap_texture_pack passing on my end

-------------------------------------------------------------------------------
bversion        ran_tests       ran     skips   failed  errors
-------------------------------------------------------------------------------
(4.1.1)         all_tests       68      2       1       test_world_import_cmcobj_jmc_full [test_mappings]
tests took 37s to run, ending with code 1

I made an error when adapting textureswap to the new system, where it wouldn't show if WorldExporter.Unknown was returned (when it should have been None). Here's what my change looks like:

diff --git a/MCprep_addon/materials/prep.py b/MCprep_addon/materials/prep.py
index 260942d..f2f9e6d 100644
--- a/MCprep_addon/materials/prep.py
+++ b/MCprep_addon/materials/prep.py
@@ -423,7 +423,7 @@ class MCPREP_OT_swap_texture_pack(
 
 	@classmethod
 	def poll(cls, context):
-		if world_tools.get_exporter(context) != world_tools.WorldExporter.Unknown:
+		if world_tools.get_exporter(context) is not None:
 			return util.is_atlas_export(context)
 		return False

[StandingPadAnimations]

Pushed my fixes for 2 of the tests, now we should have only one failing test remaining

[StandingPadAnimations]

By the way, how would one go about fixing the mappings? I'm assuming we have an automated script that just needs to be modified, but I'm not sure where to start with that

[StandingPadAnimations]

I'll revert the changes related to get_exporter

[TheDuckCow]

Making progress on the mapping issue - it's because, sometime since the version of jmc2obj I used to export our test world, exports started to be prefixed with "minecraft_block-" which was throwing off my test code for checking mappings.

Thing is I do check for that already in code so could be the test wasn't comprehensive enough, I'm checking further to see if there's a common prefix-stripping function we're using in code, so that this mapping check file works equally.

Also we had an arbitrary threshold of 30 keys not mapped to canonical texture names - I'm updating that threshold to be 40, since these pretty much seem reasonable to not be mapped (in that they are kind of special textures, not typical blocks). Point was never to have 100% coverage, but just a sanity check to help make sure we have mostly overlapping mappings.

['jmc2obj_banner-banner_base0', 'jmc2obj_banner-banner_base14', 'jmc2obj_banner-banner_base3', 'jmc2obj_banner-banner_base9', 'jmc2obj_block-redstone_dust_dot', 'jmc2obj_block-redstone_dust_line0', 'jmc2obj_block-redstone_dust_line0_on', 'minecraft_entity-armorstand-wood', 'minecraft_entity-bed-red', 'minecraft_entity-chest-ender', 'minecraft_entity-chest-normal', 'minecraft_entity-chest-normal_left', 'minecraft_entity-chest-normal_right', 'minecraft_entity-chest-trapped', 'minecraft_entity-chest-trapped_left', 'minecraft_entity-chest-trapped_right', 'minecraft_entity-creeper-creeper', 'minecraft_entity-end_portal', 'minecraft_entity-player-wide-steve', 'minecraft_entity-signs-acacia', 'minecraft_entity-signs-birch', 'minecraft_entity-signs-crimson', 'minecraft_entity-signs-dark_oak', 'minecraft_entity-signs-jungle', 'minecraft_entity-signs-mangrove', 'minecraft_entity-signs-oak', 'minecraft_entity-signs-spruce', 'minecraft_entity-signs-warped', 'minecraft_entity-skeleton-skeleton', 'minecraft_entity-skeleton-wither_skeleton', 'minecraft_entity-zombie-zombie', 'minecraft_painting-kebab', 'minecraft_painting-pool', 'minecraft_painting-void']

The separation here could actually be useful as these are mostly special entities which we might not want to accidentally overlap with block textures, something right now we don't have great control over. But, nothing to block this PR.

[TheDuckCow]

Ok I think this 'otta wrap this branch up! All tests are passing 🎉 but, there are a couple outstanding issues worth addressing before the release:

• Find missing textures is not really working well, at least for jmc2obj. I will review in another branch as I've started troubleshooting
• When all textures are missing, Prep Materials also wrongly says "no materials found to modify", which kind of doesn't make sense - even if textures are missing, it should be able to prep them (and per above, if the tick box is selected, should replace the missing textures)
• Blender 4.2 has an error when trying to use the geo nodes weather effects, see:

mcprep_addon dev response (requests lib) {'name': '-O19oCElybIt7FYFJ2bf'} at /1/track/usage_dev.json
Traceback (most recent call last):
  File "/Users/patrickcrawford/Library/Application Support/Blender/4.2/scripts/addons/MCprep_addon/tracking.py", line 887, in wrapper
    res = function(self, context)
          ^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/patrickcrawford/Library/Application Support/Blender/4.2/scripts/addons/MCprep_addon/spawner/effects.py", line 1057, in execute
    add_geonode_area_effect(context, effect)
  File "/Users/patrickcrawford/Library/Application Support/Blender/4.2/scripts/addons/MCprep_addon/spawner/effects.py", line 113, in add_geonode_area_effect
    geo_update_params(context, effect, geo_mod)
  File "/Users/patrickcrawford/Library/Application Support/Blender/4.2/scripts/addons/MCprep_addon/spawner/effects.py", line 469, in geo_update_params
    geo_mod[geo_inp_id[inp.name]] = value
    ~~~~~~~^^^^^^^^^^^^^^^^^^^^^^
TypeError: Cannot assign a 'float' value to the existing 'Input_18' Boolean IDProperty

I think all of these should be handled in their own, bespoke specific branches so we don't get hung up on any one individually in case we can't easily resolve.

[TheDuckCow]

Approving my own work from my side lol but let me know @StandingPadAnimations if you're good with the plan to merge this now, or if there's anything outstanding (related to CommonMCOBJ specifically)

[StandingPadAnimations]

Merging now, I'll create a new RC release for this