GLTF 2.0 addon
ClosedPublic
Actions

Authored by Julien DUROURE (julien) on Nov 10 2018, 1:23 PM.

Details

Reviewers

Campbell Barton (campbellbarton)
Bastien Montagne (mont29)

Commits

rBAb1f2133fa284: Initial commit of glTF 2.0 importer/exporter

Summary

Importer / Exporter of gltf2 files.

This addon is developed by KhronosGroup.

Here is current development git & documentation:
https://github.com/KhronosGroup/glTF-Blender-IO

For long term development and maintenance, we will have daily changes and fixes in the github repository. If we have a major and/or critical update, we also want to provide and integrate it into future versions of Blender.

Diff Detail

Event Timeline

@Julien DUROURE (julien) Please commandeer the revision, so that I can get back to reviewer status ;)

Julien DUROURE (julien) commandeered this revision.Nov 10 2018, 1:31 PM

Julien DUROURE (julien) added a reviewer: Bastien Montagne (mont29).

Ed Mackey (emackey) added a subscriber: Ed Mackey (emackey).Nov 12 2018, 4:06 PM

Don McCurdy (donmccurdy) added a subscriber: Don McCurdy (donmccurdy).Nov 13 2018, 7:47 PM

Mitchell Stokes (moguri) added a subscriber: Mitchell Stokes (moguri).Nov 15 2018, 12:36 AM

Only really checked to top-level 'add-on' part. The whole thing is waaaayyyy too huge, just did very quick skimming over it and saw nothing obviously wrong… So I think that can go to add-on repo, maybe @Campbell Barton (campbellbarton) also wants to have a look though?

./io_scene_gltf2/__init__.py
35	Should be merged with line 21 (one way or the other).
450–469	Why two different operators? format could be a simple Enum property of a single operator, no need for two here.
472–477	Same as above, only need one function here, registering to entries for single operator, setting the format parameter to both values (binary and ascii).
522	Just do that in execute() ? Why the extra level here?
./io_scene_gltf2/blender/exp/gltf2_blender_export.py
32–33	You could use `ProgressReport` from `bpy_extras.wm_utils.progress_report` (see OBJ add-on for an example usage). Not mandatory of course, but nice to report some status messages in console during long imports. ;)

This revision is now accepted and ready to land.Nov 19 2018, 10:44 AM

Looked over this patch, and there are some minor API usage issues (none of them blockers though).

Mentioning them because it would be good to resolve them, even if not in initial commit.

General Comments

Would prefer all classes use __slots__ unless they need dynamically defined attributes.

With such large scripts and multiple contributors, the chance a typo in attribute assignment goes by unnoticed gets higher.

Classes used as containers for static/class methods should disallow instantiation:

def __new__(cls, *args, **kwargs):
    raise RuntimeError("%s should not be instantiated" % cls)

./io_scene_gltf2/__init__.py
70–71	No need for `get` call, just do: `context.scene.pop(operator.scene_key)`
309	You can avoid boiler plate code here by using `self.as_keywords()` here, and matching up names.
504–508	No need to define separate variable.
523	no need to use `bpy.context`
./io_scene_gltf2/blender/com/gltf2_blender_conversion.py
18	Wouldn't bother with class here, this is already a module. If it's needed to have a sub-module we could have `gltf2_blender_conversion/conversion.py`
./io_scene_gltf2/blender/com/gltf2_blender_image_util.py
62–63	These should be restored after overwriting.
93–94	better do: `width, height = blender_image.size`
./io_scene_gltf2/blender/com/gltf2_blender_math.py
30–43	use `elif`.
./io_scene_gltf2/blender/exp/gltf2_blender_export.py
32	again, this takes `context`, no need for `bpy.context`
./io_scene_gltf2/blender/exp/gltf2_blender_gather_materials_pbr_metallic_roughness.py
39	Double underscore should be reserved for functions which Python knows about, for private API's use single.

Hello,

Thanks for your feedbacks.
We are going to handle them.

What's the plan now?
Should we update the code (using Update diff in phabricator), or are you going to commit the code, and we will push cleanups after?
We already have some bugfixs that we can commit if you give me write access on repo.

Thanks again,

Julien

Hey @Julien DUROURE (julien), best make the changes and update the diff.

If some of these need more development then can be done in a week or two,
it's fine to note them as TODO 's in the code.

Mike Erwin (merwin) added a subscriber: Mike Erwin (merwin).Nov 21 2018, 7:14 AM

Hi @Campbell Barton (campbellbarton) and @Bastien Montagne (mont29) ,

I updated the diff with most of your feedback.

@Julien DUROURE (julien), I've given you commit access to the addons repository, for when you're ready to commit. More information on how to set things up is here:
https://wiki.blender.org/wiki/Tools/Git#Commit_Access
https://wiki.blender.org/wiki/Process/Addons

Closed by commit rBAb1f2133fa284: Initial commit of glTF 2.0 importer/exporter (authored by Julien DUROURE (julien)). · Explain WhyNov 24 2018, 4:34 PM

This revision was automatically updated to reflect the committed changes.

Hi all, may I ask a process question here? I'm curious what's the best strategy to keep development efforts synchronized between here and the Khronos GitHub repository. I believe the plan is for issues and patches to continue to be managed on GitHub, unless we create administrative problems in so doing.

If we do keep the Khronos GitHub going (as I think we should), we would want to know which version of the GitHub repo was the most recent one contributed here (via version tag or even git SHA1 from the Khronos side). Likewise we'd want to be aware of any patches landing here that have not found their way back to GitHub, so they can show up in a Pull Request there. Thoughts?

Apologies if I've asked in the wrong place. We could make an issue for this on the GitHub side of course, but I wanted to get feedback from the Blender devs who may have experience with external contributors keeping their own source control systems around.

I think we've always left this kind of synchronization up to the addon maintainers. Maybe others have some advice for how to manage things.

It would be good if we could mark this an Official (rather than Community) addon, enabled by default. I think this file format is important enough for that.

One thing that's confusing in the UI though is that the export menu has separate entries for .gltf and .glb. Is this really needed, is there a specific reason for it? I rather we have just one with the most common choice enabled by default. The name in the menu and the tooltips give the user also no information about what the difference is and why they might choose one or the other.

Hi @Brecht Van Lommel (brecht),

I will update from COMMUNITY to OFFICIAL upstream, and push here.
For enabling by default, can I let you manage it? I'm not sure how to do it/if I can do it

Regarding menu entries, it's tag on TODO on code, this will probably be changed after beta

I think we indeed need to do it in the Blender repository.

Can you apply this patch before that?

P832 (An Untitled Masterwork)

1	commit c724849d91eeb3c6eb080272ce1de8e87af344c6
2	Author: Brecht Van Lommel <brechtvanlommel@gmail.com>
3	Date: Sat Nov 24 20:35:30 2018 +0100
4
5	Fixes for glTF:
6
7	* Explain difference between .gltf and .glb export in UI.
8	* Fix startup warning in console about tooltip.
9	* Lazy import modules for performance.
10
11	diff --git a/io_scene_gltf2/__init__.py b/io_scene_gltf2/__init__.py
12	index 0e864cad..ebfcb50d 100755
13	--- a/io_scene_gltf2/__init__.py
14	+++ b/io_scene_gltf2/__init__.py
15	@@ -21,9 +21,7 @@ import bpy
16	from bpy_extras.io_utils import ImportHelper, ExportHelper
17	from bpy.types import Operator, AddonPreferences
18
19	-from .io.com.gltf2_io_debug import print_console, Log
20	-from .io.imp.gltf2_io_gltf import glTFImporter
21	-from .blender.imp.gltf2_blender_gltf import BlenderGlTF
22	+from .io.com.gltf2_io_debug import Log
23
24	from bpy.props import (CollectionProperty,
25	StringProperty,
26	@@ -198,7 +196,7 @@ class ExportGLTF2_Base:
27
28	export_frame_step: IntProperty(
29	name='Frame step size',
30	- description='Step size (in frames) for animation export.',
31	+ description='Step size (in frames) for animation export',
32	default=1,
33	min=1,
34	max=120
35	@@ -461,9 +459,9 @@ class ExportGLTF2_GLTF(bpy.types.Operator, ExportGLTF2_Base, ExportHelper):
36
37
38	class ExportGLTF2_GLB(bpy.types.Operator, ExportGLTF2_Base, ExportHelper):
39	- """Export scene as glTF 2.0 file"""
40	+ """Export scene as binary glTF 2.0 file"""
41	bl_idname = 'export_scene.glb'
42	- bl_label = 'Export glTF 2.0 binary'
43	+ bl_label = 'Export Binary glTF 2.0'
44
45	filename_ext = '.glb'
46	filter_glob: StringProperty(default='*.glb', options={'HIDDEN'})
47	@@ -471,12 +469,9 @@ class ExportGLTF2_GLB(bpy.types.Operator, ExportGLTF2_Base, ExportHelper):
48	export_format = 'BINARY'
49
50
51	-def menu_func_export_gltf(self, context):
52	+def menu_func_export(self, context):
53	self.layout.operator(ExportGLTF2_GLTF.bl_idname, text='glTF 2.0 (.gltf)')
54	-
55	-
56	-def menu_func_export_glb(self, context):
57	- self.layout.operator(ExportGLTF2_GLB.bl_idname, text='glTF 2.0 (.glb)')
58	+ self.layout.operator(ExportGLTF2_GLB.bl_idname, text='Binary glTF 2.0 (.glb)')
59
60
61	class ExportGLTF2_AddonPreferences(AddonPreferences):
62	@@ -521,6 +516,9 @@ class ImportglTF2(Operator, ImportHelper):
63	return self.import_gltf2(context)
64
65	def import_gltf2(self, context):
66	+ from .io.imp.gltf2_io_gltf import glTFImporter
67	+ from .blender.imp.gltf2_blender_gltf import BlenderGlTF
68	+
69	import_settings = self.as_keywords()
70
71	self.gltf_importer = glTFImporter(self.filepath, import_settings)
72	@@ -562,8 +560,7 @@ def register():
73	# bpy.utils.register_module(__name__)
74
75	# add to the export / import menu
76	- bpy.types.TOPBAR_MT_file_export.append(menu_func_export_gltf)
77	- bpy.types.TOPBAR_MT_file_export.append(menu_func_export_glb)
78	+ bpy.types.TOPBAR_MT_file_export.append(menu_func_export)
79	bpy.types.TOPBAR_MT_file_import.append(menu_func_import)
80
81
82	@@ -573,7 +570,6 @@ def unregister():
83	# bpy.utils.unregister_module(__name__)
84
85	# remove from the export / import menu
86	- bpy.types.TOPBAR_MT_file_export.remove(menu_func_export_gltf)
87	- bpy.types.TOPBAR_MT_file_export.remove(menu_func_export_glb)
88	+ bpy.types.TOPBAR_MT_file_export.remove(menu_func_export)
89	bpy.types.TOPBAR_MT_file_import.remove(menu_func_import)
90