Move to current viewport transform when updating scene #263
Conversation
keianhzo
changed the title
| from mathutils import Matrix, Vector, Euler | ||
| from math import radians |
There was a problem hiding this comment.
Why are these imports mixed into the code here?
There was a problem hiding this comment.
I don't know of any reason why locally imported modules are bad (if there is any let me know). I usally import globally for modules that are widely used and locally for modules that are specific to a function. There are quite a lot of cases of this throught the code.
There was a problem hiding this comment.
Locally imported modules aren't bad per se, but (in my opinion) keeping imports at the top makes it easier to see what a file depends on, gives a consistent spot to look for this information, and prevents duplication. Sometimes they have to be imported in the middle of code to resolve dependency issues or other bugs and that's what I thought was usually the reason they had been added throughout the code (and that's the main reason I asked, because I wondered if you were working around a bug). In certain cases they may cause a slowdown in the middle of execution instead of at the beginning (but that's likely not an issue here). Also, PEP8 does specify for imports to be placed at the top: https://peps.python.org/pep-0008/#imports
This page also gives some reasons for placing them at the top: https://www.quora.com/Should-import-be-above-all-from-statements-in-Python-If-yes-why
| bpy.ops.object.empty_add(location=final_loc, rotation=(euler.x, euler.y, euler.z), type="ARROWS") | ||
| viewpoint = bpy.context.object | ||
| viewpoint.name = "__scene_debugger_viewpoint" | ||
| from .components.utils import add_component |
There was a problem hiding this comment.
As above, why is this import mixed into the code here?
There was a problem hiding this comment.
Same as above. Also for imports among our own file I try to do local importas to avoid circular dependencies.
addons/io_hubs_addon/debugger.py
Outdated
| final_loc = loc + Vector((0, 0, -1.6)) | ||
| rot_offset = Matrix.Rotation(radians(180), 4, 'Z').to_4x4() | ||
| final_rot = rot.to_matrix().to_4x4() @ rot_offset | ||
| euler: Euler = final_rot.to_euler() |
There was a problem hiding this comment.
Why is this annotated? (Nothing else is annotated here)
There was a problem hiding this comment.
Yeah we don't really do that a lot, i can remove it although I don't think it hurts.
There was a problem hiding this comment.
Thanks. And yeah, it doesn't hurt, but I don't think it adds anything here either and it's inconsistent.
| hubs_session.move_to_waypoint("__scene_debugger_viewpoint") | ||
| ob = bpy.context.scene.objects["__scene_debugger_viewpoint"] |
There was a problem hiding this comment.
I think these should use viewpoint.name
There was a problem hiding this comment.
Not sure if I follow. We are checking if the object in line 65 exists or not, if we check for the attribute that will throw an exception.
There was a problem hiding this comment.
Ah, sorry. I wasn't clear. I wasn't talking about the if check, I was referring to the two lines below it that use the hard coded version of viewpoint.name ("__scene_debugger_viewpoint"), if you use viewpoint.name as the arguments then if that ever needs changing there's only one spot to change it.
| return {'FINISHED'} | ||
| except Exception as err: | ||
| print(err) | ||
| bpy.ops.wm.hubs_report_viewer('INVOKE_DEFAULT', title="Hubs scene debugger report", report_string='\n\n'.join( | ||
| ["The scene export has failed", "Check the export logs or quit the browser instance and try again", f'{err}'])) | ||
|
|
||
| if viewpoint: | ||
| ob = bpy.context.scene.objects["__scene_debugger_viewpoint"] |
There was a problem hiding this comment.
As above, I think this should use viewpoint.name
| row = box.row() | ||
| row.prop(context.scene.hubs_scene_debugger_room_export_prefs, "avatar_to_viewport") |
There was a problem hiding this comment.
This seems to not do anything when spawning as an object (which is what I'd expect), so what do you think about greying it out?
if "debugLocalScene" not in hubs_session.room_params:
row.active = False
There was a problem hiding this comment.
Good idea!. I'll add that.
addons/io_hubs_addon/hubs_session.py
Outdated
| try { | ||
| setTimeout(() => { | ||
| window.location = `${window.location.href}#${arguments[0]}`; | ||
| const mat = APP.world.scene.getObjectByName("__scene_debugger_viewpoint").matrixWorld; |
There was a problem hiding this comment.
I think this should also use ${arguments[0] instead of hard coding __scene_debugger_viewpoint
There was a problem hiding this comment.
Yep, good call. Updated.
addons/io_hubs_addon/utils.py
Outdated
| @@ -175,3 +175,14 @@ def load_prefs(context): | |||
| new_room = prefs.hubs_rooms.add() | |||
| new_room.name = room["name"] | |||
| new_room.url = room["url"] | |||
|
|
|||
|
|
|||
| def find_area(): | |||
There was a problem hiding this comment.
I think this should take the area type and would make a good generic function.
This PR adds support for scene update moving the avatar to the same position as the current Blender 3D view transform so you can focus on specific details of the scene without needing to move there after the scene is updated. This does not change the head orientation, that will stay the same as it was before, it only moves the avatar to the same position as the 3D viewport camera.
keianhzo
force-pushed
the
spawn-on-viewport-transform
branch
from
5f8b227 to
efbe8fd
Compare
This PR adds support for scene update moving the avatar to the same position as the current Blender 3D view transform so you can focus on specific details of the scene without needing to move there after the scene is updated.
This does not change the head orientation, that will stay the same as it was before, it only moves the avatar to the same position as the 3D viewport camera.