Model JSON tweaks #469
Model JSON tweaks #469
Conversation
|
Should be ready for review now. |
|
Should we move enum into one file because those are scattering everywhere now? |
|
I need to do more of a review, but seeing we're getting into the nitty details of code style makes me think we're pretty close on this, and as I understand would greatly increase the reliability of being able to use the model importer (our top source of errors from users). As such, I've marked this for the 3.5.0 milestones, in the hopes we can wrap this up in the next week or two max to merge. |
MCprep_addon/spawner/mcmodel.py
Outdated
| r, obj = add_model(os.path.normpath(self.filepath), name) | ||
| if r: | ||
| self.report( | ||
| {"ERROR"}, "The JSON defined is not valid for a Minecraft Java Edition model") |
There was a problem hiding this comment.
| {"ERROR"}, "The JSON defined is not valid for a Minecraft Java Edition model") | |
| {"ERROR"}, "The JSON model is not avalid JSON model as per Minecraft Java Edition standards") |
There was a problem hiding this comment.
How about "The JSON model is not valid for Minecraft Java Edition"
Or something like "The JSON model does not contain any geometry elements" since it only return that because it doesn't have any geometry information.
MCprep_addon/spawner/mcmodel.py
Outdated
| r, obj = add_model(os.path.normpath(self.filepath), filename) | ||
| if r: | ||
| self.report( | ||
| {"ERROR"}, "The JSON defined is not valid for a Minecraft Java Edition model") |
There was a problem hiding this comment.
| {"ERROR"}, "The JSON defined is not valid for a Minecraft Java Edition model") | |
| {"ERROR"}, "The JSON model is not avalid JSON model as per Minecraft Java Edition standards") |
There was a problem hiding this comment.
Couple of issues I noticed with enums and comments. Otherwise, mostly good.
I did see an instance where MCprep will force the user to use meshswap models over JSON models. That is unacceptable, the precedent is that we do not decide on the user's behalf anything related to imports unless it fixes an actual issue (Non-standard color spaces for OBJs, feature compatibility, etc.). Nowhere does that mean, “let's ignore the user's wish to use a JSON model because we already have a meshswap model”.
The issue is certain Minecraft "block" isn't really a normal block like most of the blocks in the Block model. Most of the blocks in #267 would be called as an entity block/tile and some cases like double block flower, tall grass... would it better to be a one mesh block through meshswap |
|
I originally I would make fixed json version of those entity blocks but blend file would be better suitable for it, I'm expecting "why this shulker block, chest in block model can't do that open intersction and meshswap has another version of them that can open, interact" |
If a user imports a JSON model and expects to have all entity interactions, that's an issue with the user not knowing how to use MCprep, not an issue where MCprep has to spoonfeed users the quote on quote "correct" model (Especially because we already have a meshswap spawner). For Steve's sake, it's called, "Import JSON Model", not, "Import JSON Model But We Force Meshswap Models for Certain Blocks". The latter sounds ridiculous, I know, but that's what's being implemented when we start assuming what the user wants. What about the users that want blocks without any interaction features? (i.e. a regular block) Do we start telling them, "Sorry, but we don't want to give users the inconvinience of troubleshooting because of their mistake, so you'll have edit the MCprep code to properly import JSON models"? Sounds ridiculous, but again, that's what happens when we start to assume. (I would love to quote one of my former architecture teachers regarding assuming, but it involves some language not really fit on the GitHub repo for a Minecraft animation workflow addon) The whole idea behind importing JSON models in MCprep is to import a JSON model as is, without any additional features. Forcing meshswap models in JSON import defeats the purpose of JSON imports (again, we already have a meshswap spawner). |
|
Why would they expecting an interaction while those blocks only the entity tile blocks in the game. How would they import a modded entity block? Since they don't even have any geometry data inside it would be empty because entity block is through hardcoded model renderer (now there is Geckolib, we don't even support Bedrock format so Geckolib is not too) I know this is the hard one because of the terminology we use for this. #148 |
|
If no geometry is defined in JSON models for entities, then it's simply more transparent to not include entities in the JSON importer. It's both disingenuous and frustrating to import meshswap models in the JSON importer. At this point, it's a decision between transparency vs. obscure behavior, which raises a red flag. If that means having to put a notice in the UI saying “Entities are under the Meshswap importer” (though I also see an entity spawner, so it should be obvious anyway), so be it. I'd rather have the JSON importer be transparent in what it can import than have to debug for hours wondering why it's importing undesired meshswap models, only to find that it's forced behavior. At this route, I'd say: I get it, we want to include every JSON model, but transparent behavior is preferred. |
|
So first want to acknowledge that this is a tricky subject, and indeed there's no singular correct answer. I do appreciate though the focus on what will make sense for users, and not obscuring functionality or extending lower level functions in ways that are not expected. Copying some of my comments from discord, I could see us going a route that is more like creating a meta or uber operator that clearly indicators it's "doing multiple things", with lower level operations that can also be performed individually.
These are just some high level thoughts. In terms of the scope of this PR: we are talking abotu Model JSON tweaks, so let's try to keep the scope tight and just focus on the core changes. It becomes problematic if we start making each PR try to cover many different things, as any conversation like this derails reviewing and merging of earlier changes that would otherwise be good to go. So in that sense, I'd like to ask - is there anything else in progress here? Or are we good to review with the intention that approval would mean as-is merging? Asking @zNightlord thanks! |
|
Yeah it should be ready for another review For the modded block entity case, that is just an example for "Import JSON model" there are other json model structure that user would think they can import and have modded block geometry and rigged bones, animation. |
Adding blocks in Meshswap
|
Mergind into dev |
Previously #453
This should take care of that issue in #457, I hope.
Need to find a nice way to check the namespace "minecraft" vanilla, no longer since there are cases can't really find minecraft vanilla textureUse default no diffuse texture setup instead.