Added mcmodel importer #168
Conversation
There was a problem hiding this comment.
Submitting my first review here! We might have a couple iterations, and I think the macro idea of "what to name this file, and whether to make json object loading a more generically usable function" is still something I want to think about. But these comments are a good starting place for now.
MCprep_addon/spawner/mcmodel.py
Outdated
|
|
||
| from bpy_extras.io_utils import ImportHelper | ||
|
|
||
| import json |
There was a problem hiding this comment.
Typically standard python imports go first, followed by libraries used by the project (in our case, all of the bpy or blender specific imports), and finally followed by the local relative imports (e.g. from .. import conf).
Also one convention in I've been using that would be good to keep consistent is not doing from bpy.props import *, but just being explicit when defining properties (ie prop = bpy.types.StringProperty())
MCprep_addon/spawner/mcmodel.py
Outdated
| bl_options = {'REGISTER', 'UNDO'} | ||
|
|
||
| filename_ext=".json" | ||
| filter_glob: StringProperty( |
There was a problem hiding this comment.
Compatibility callout: Because MCprep still supports older blender versions (down to blender 2.72), we actually need to still use the syntax filter_glob = bpy.types.StringProperty instead of the colon. This is because it's not recognized syntax in the versions of python shipped with older blender versions.
To ensure these properties are still annotated to avoid 2.8+ console warnings, MCprep uses a wrapper function during registration. See the example here how this is used (and the related relative file import you'll need to make)
MCprep_addon/spawner/mcmodel.py
Outdated
| with open(self.filepath, 'r') as f: | ||
| obj_data=json.loads(f.read()) | ||
|
|
||
| directories = self.filepath.split('\\') |
There was a problem hiding this comment.
Flagging as looks suspicious to me - using os.sep instead to would make this one line more cross compatible between windows and linux, as hard coding something like / (linux/mac) or \ (windows) breaks compatibility. os.sep is used to replace such OS-specific path folder separators.
But beyond that niche point, both these two lines could be simplified (if I'm reading correctly) by using filename = os.path.basename(self.filepath)
This would keep the extension, so if you need to strip of the .json for instance, then you could furthermore use filename_no_ext = os.path.splitext(filename)[0] (where index [1] would be the .json part).
You could also combine this into one line, which starts to be a little complex for a single line of code but I think is still ok since it's fairly readable:
filename = os.path.splittext(os.path.basename(self.filepath))[0]
MCprep_addon/spawner/mcmodel.py
Outdated
| return [verts,edges,faces] | ||
|
|
||
| def add_material(name="material", pth=""): | ||
| print(name+": "+pth) |
There was a problem hiding this comment.
MCprep specific paradigm: use conf.log(name+": "+pth), so that print respect the state of the UI "Verbose" setting. Must run from .. import conf first, under the local relative imports at the top of the file.
MCprep_addon/spawner/mcmodel.py
Outdated
| textures = object.get("textures") # currently only looks for textures local to file, need to implement namespace ID with resource packs. | ||
| for img in textures: | ||
| if img!="particle": | ||
| tex_file=textures[img].split('/')[len(textures[img].split('/'))-1] |
There was a problem hiding this comment.
Flagging for cross OS compatibility: hard coded / will break, better to use the character defined by os.sep, though better than that would be using more direct path functions under os.path where possible (made a comment later that is similar)
| # make the bmesh the object's mesh | ||
| bm.to_mesh(mesh) | ||
| bm.free() | ||
|
|
There was a problem hiding this comment.
Style guide 'nit-pick' comment: Enter two lines of spaces between top level functions and classes (which needs to be added), and only one newline between functions and methods within classes or functions (which you already do).
MCprep_addon/spawner/mcmodel.py
Outdated
| )) | ||
|
|
||
| def add_element(elm_from=[0,0,0],elm_to=[16,16,16],rot_origin=[8,8,8],rot_axis='y',rot_angle=0): | ||
| # this function calculates and defines the verts, edge, and faces that will be created |
There was a problem hiding this comment.
Halfway to a docstring!
See the standard way to [document functions here[(https://www.python.org/dev/peps/pep-0257/#id16), better to close around triple quotes than to make it a comment. There's plenty of inconsistency in MCprep around this, but I'm trying to move it towards this standard over time.
If it goes over 1 line of code, better to make the first line fit within 80 characters, and then make a blank line, and then on another line write more details (or, describe the arguments).
|
Let me know if you need any other help or direction on how to review these comments/the changes requested. |
|
Sounds good, everything makes sense and I've already started making most of the changes, just been fairly busy with class work and exams so I didn't really have to time to finish. |
|
No worries, no rush here - I'm planning a micro release update in the next week so nothing new will be merged before then, beyond that I'm hoping for a release late Nov or early Dec. If we wanted to include any of the features here in that, we'd likely want to try and merge by end of Nov at the latest. But no pressure if not in by then, just a consideration! |
Default minecraft models can now properly be imported. Model parents will also be read and the child will properly overwrite whats needed. Requires assets/models to be in the resource pack. Models with multiple textures now create each material and apply them to the proper faces.
MCprep_addon/spawner/mcmodel.py
Outdated
| new_pos[(1+axis_i)%3]=cos(r)*(a-m)+(b-n)*sin(r)+m | ||
| new_pos[(2+axis_i)%3]=-sin(r)*(a-m)+cos(r)*(b-n)+n | ||
| new_pos[(3+axis_i)%3]=c | ||
| ''' offset and scale are applied to the vertices in the return |
There was a problem hiding this comment.
Nitpick: How you had it before is actually better, using # for multi line comments within blocks of code (see here)
MCprep_addon/spawner/mcmodel.py
Outdated
|
|
||
| return [verts,edges,faces] | ||
|
|
||
| def add_material(name="material", pth=""): # creates a simple material with an image texture from pth |
There was a problem hiding this comment.
Comments are good! But here, may as well turn it into a proper "function docstring". Put onto a new line, and surround with triple double quotes.
https://www.python.org/dev/peps/pep-0008/#documentation-strings
MCprep_addon/spawner/mcmodel.py
Outdated
|
|
||
| def read_model(model_filepath): # recursive function that extracts the information from the json files | ||
| with open(model_filepath, 'r') as f: | ||
| obj_data=json.loads(f.read()) |
There was a problem hiding this comment.
Better to use json's built in load method for file operators. So just write obj_data=json.load(f) (notice it's load, instead of loads).
MCprep_addon/spawner/mcmodel.py
Outdated
|
|
||
| def locate_image(textures,img,model_filepath): | ||
|
|
||
| context=bpy.context |
There was a problem hiding this comment.
Is it possible to pass context into the function directly, instead of calling for bpy.context here?
Same applies to other cases where you immediately define context = bpy.context
| if parent=="builtin/generated" or parent=="item/generated": | ||
| pass # generates the model from the texture | ||
| elif parent=="builtin/entity": | ||
| pass # model from an entity file, only for chests, ender chests, mob heads, shields, banners and tridents |
There was a problem hiding this comment.
Thanks for the comment details here! Is this list exhaustive, or are there some other examples too? (if others, just end it with , etc.)
MCprep_addon/spawner/mcmodel.py
Outdated
| faces = e.get("faces") | ||
| for i in range(len(element[2])): | ||
| f=element[2][i] | ||
| if faces: |
There was a problem hiding this comment.
To avoid a few of these multiple levels of if indenting, you can invert the logic with a continue statement. Generally it's best for readability to keep your levels of indentation as low as possible! Won't work for all of them, but will help.
if not faces:
continue
d_face=faces.get(face_dir[i])
if not d_face:
continue
face=bm.faces.new(...
MCprep_addon/spawner/mcmodel.py
Outdated
| if uv_cords is None: | ||
| uv_cords=[0,0,16,16] | ||
|
|
||
| uvs=[ |
There was a problem hiding this comment.
Could you add a comment here describing (one-liner or two liner at most, if possible) the logic behind these indices? e.g. why is it 2,0,0,2 and 1,1,3,3, and functionally what is this subtraction representing/doing?
MCprep_addon/spawner/mcmodel.py
Outdated
| ] | ||
|
|
||
| for j in range(len(face.loops)): | ||
| face.loops[j][uv_layer].uv = uvs[(j+uv_idx)%len(uvs)] |
There was a problem hiding this comment.
For my understanding - is %len(uvs) to cause the uv to wraparound?
MCprep_addon/spawner/mcmodel.py
Outdated
|
|
||
| mesh.uv_layers.new() | ||
| uv_layer = bm.loops.layers.uv.verify() | ||
| elements, textures = read_model(model_filepath) |
There was a problem hiding this comment.
As per other comment, would be nice to pass in context as a first variable here into read_model; will require passing context in from the parent operator call, where it is readily available.
MCprep_addon/spawner/mcmodel.py
Outdated
| directory = os.path.join(resource_folder,"assets",namespace,"textures") | ||
| return os.path.realpath(os.path.join(directory,local_path)+".png") | ||
|
|
||
| def read_model(model_filepath): # recursive function that extracts the information from the json files |
There was a problem hiding this comment.
Thanks for calling out the recursive nature here! Much like the other, might move this to a standard docstring, and add a subparagraph within it explaining why it needs to be recursive (as I understand it: each json file references other json files, so we need to recursively load other resources as we walk through)
Went through and made the reccommended changes. Context is now passed as a function parameter for locate_image and read_model. Added commets that describe how the uv coords work. I think before I was a bit confused of the concept of docstrings but I believe I better understand it and hopefully implemented it correctly.
|
Hey just checking back in, with the recent MCprep release, one of my next targets is to get this into Blender. Let me know if there's any other changes you want to make, or if you want to respond to some of the prior feedback. I'm also happy to take it from here and marge when I feel ready. |
|
Long overdue, but with v3.2.4.2 released now, I'm going to be focussing on merging this in. Happy to take it from here as needed. |
|
Hey, just an FYI I'm going to be prioritizing to merge this into the v3.2.6 release, as per milestones below: Let me know if you want to be further involved, but by default I can take it from here. Thanks again for the help getting it to this stage. |
|
I'm going to go ahead and merge this into dev as is and do some cleanup after - apologies @TimyAnimations it took me this long to get back to, but excited to have this ready! Goal is before New Years. |
No description provided.