-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for scene debugger #249
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just an initial portion of my review, I will post more later.
Installing the dependencies seems prone to breakage depending on Blender/Python version, e.g. I got this when trying to install the dependencies in Blender 2.93
/home/user/Workshop/CustomApps/blender-2.93.17/2.93/python/bin/python3.9: No module named pip
.
Blender 2.93 uses Python 3.9 while later Blenders use Python 3.10. Also, no indication was given to the user that installing the dependency had failed.
Trying to install it in Blender 3.6 worked better, but produced this warning at the end:
Installing collected packages: sortedcontainers, urllib3, sniffio, pysocks, idna, h11, exceptiongroup, certifi, attrs, wsproto, outcome, trio, trio-websocket, selenium
ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
requests 2.27.1 requires urllib3<1.27,>=1.21.1, but you have urllib3 2.1.0 which is incompatible.
Successfully installed attrs-23.1.0 certifi-2023.11.17 exceptiongroup-1.2.0 h11-0.14.0 idna-3.4 outcome-1.3.0.post0 pysocks-1.7.1 selenium-4.15.2 sniffio-1.3.0 sortedcontainers-2.4.0 trio-0.23.1 trio-websocket-0.11.1 urllib3-2.1.0 wsproto-1.2.0
Continuing on, I tried to create a room, and that worked fine (once I'd switched it from the default hubs.local instance URL. I think this should default to the demo instance to make it easier for people).
Then when I tried to update the room it errored out with an error about not being able to find the exported GLB file:
11:01:40 | INFO: Draco mesh compression is available, use library at /home/user/Workshop/CustomApps/blender-3.6.5/3.6/python/lib/python3.10/site-packages/libextern_draco.so
[Errno 13] Permission denied: '/Users'
Traceback (most recent call last):
File "/home/user/Workshop/Programming/Blender/user_scripts_dir/addons/io_hubs_addon/components/ui.py", line 235, in execute
refresh_scene_viewer()
File "/home/user/Workshop/Programming/Blender/user_scripts_dir/addons/io_hubs_addon/components/ui.py", line 212, in refresh_scene_viewer
file_input.send_keys(os.path.join(bpy.app.tempdir, EXPORT_TMP_FILE_NAME))
File "/home/user/.local/lib/python3.10/site-packages/selenium/webdriver/remote/webelement.py", line 230, in send_keys
self._execute(
File "/home/user/.local/lib/python3.10/site-packages/selenium/webdriver/remote/webelement.py", line 394, in _execute
return self._parent.execute(command, params)
File "/home/user/.local/lib/python3.10/site-packages/selenium/webdriver/remote/webdriver.py", line 347, in execute
self.error_handler.check_response(response)
File "/home/user/.local/lib/python3.10/site-packages/selenium/webdriver/remote/errorhandler.py", line 229, in check_response
raise exception_class(message, screen, stacktrace)
selenium.common.exceptions.InvalidArgumentException: Message: File not found: /tmp/blender_fVu4ln/__hubs_tmp_scene_.glb
Stacktrace:
RemoteError@chrome://remote/content/shared/RemoteError.sys.mjs:8:8
WebDriverError@chrome://remote/content/shared/webdriver/Errors.sys.mjs:187:5
InvalidArgumentError@chrome://remote/content/shared/webdriver/Errors.sys.mjs:378:5
interaction.uploadFiles@chrome://remote/content/marionette/interaction.sys.mjs:543:13
Error: Python: Traceback (most recent call last):
File "/home/user/Workshop/Programming/Blender/user_scripts_dir/addons/io_hubs_addon/components/ui.py", line 235, in execute
refresh_scene_viewer()
File "/home/user/Workshop/Programming/Blender/user_scripts_dir/addons/io_hubs_addon/components/ui.py", line 212, in refresh_scene_viewer
file_input.send_keys(os.path.join(bpy.app.tempdir, EXPORT_TMP_FILE_NAME))
File "/home/user/.local/lib/python3.10/site-packages/selenium/webdriver/remote/webelement.py", line 230, in send_keys
self._execute(
File "/home/user/.local/lib/python3.10/site-packages/selenium/webdriver/remote/webelement.py", line 394, in _execute
return self._parent.execute(command, params)
File "/home/user/.local/lib/python3.10/site-packages/selenium/webdriver/remote/webdriver.py", line 347, in execute
self.error_handler.check_response(response)
File "/home/user/.local/lib/python3.10/site-packages/selenium/webdriver/remote/errorhandler.py", line 229, in check_response
raise exception_class(message, screen, stacktrace)
selenium.common.exceptions.InvalidArgumentException: Message: File not found: /tmp/blender_fVu4ln/__hubs_tmp_scene_.glb
Stacktrace:
RemoteError@chrome://remote/content/shared/RemoteError.sys.mjs:8:8
WebDriverError@chrome://remote/content/shared/webdriver/Errors.sys.mjs:187:5
InvalidArgumentError@chrome://remote/content/shared/webdriver/Errors.sys.mjs:378:5
interaction.uploadFiles@chrome://remote/content/marionette/interaction.sys.mjs:543:13
When I look in Blender's temp directory the GLB file indeed isn't there, but if I manually paste the relevant code from the export_scene
function into Blender's Python console it works fine and the file gets created in the temp directory, so I'm not sure why it's not working.
The GLB issues is a silly error about a hardcoded path that I've fixed. I'll look into the 2.93 issue, I haven't tested that much there |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After your most recent changes I was able to get it to work and it's predominantly working quite nicely. However, I've left some inline comments and I have a a few more general thoughts and comments I'll list here.
- The scene debugger code is all mixed in with the UI code, but it should really be in its own separate file. The
HUBS_PT_ToolsPanel
should stay though as it's a generic UI element. - Can an existing room be used? Or can a lighter scene be specified for room creation? Currently it takes a long time for the room to load for me for the first time because it's having to download a large scene (at least on the demo server) that I'm going to immediately overwrite.
- The install dependencies operator doesn't provide any feedback if the install fails. I think it can be really easy to miss that it's failed, especially if it fails early in the process and would benefit from an error message being reported to the user.
- The switch window/bring to front code isn't working for Firefox for me, but it works great in Chromium. Since the browser isn't using my window manager decorations I also have no option to keep it always on top to work around the issue.
'export_format': ('GLB' if extension == '.glb' else 'GLTF_SEPARATE'), | ||
'filepath': os.path.join(bpy.app.tempdir, EXPORT_TMP_FILE_NAME), | ||
'export_cameras': True, | ||
'export_lights': True, | ||
'export_extras': True, | ||
'use_visible': True, | ||
'export_apply': True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While these can be overridden by the "Remember Export Settings" checkbox in the exporter, this isn't very obvious to the user and the glTF exporter has historically not implemented its history retention in a very reliable way. I'm kinda thinking that these properties, plus potentially some of the animation ones that cause trouble should be exposed in the Hubs panel near the update button and the "Remembered" settings not used at all or maybe just use the "Remembered" settings without any overrides (although I think I prefer the first option). Or possibly the export settings that will be applied could be visualized in the panel, maybe a combination of overrides and "Remembered" settings, but the settings should be explicit to the user so they know ahead of time exactly what will be exported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this is also left over from a previous version. My idea was to totally rely on the exporter settings. Looking at this again and being the export properties saving not ituitive, as you need to export at least one to actually save, maybe it's a good idea to expose a basic set of export options at least for this initial version.
Do you know if there is an easy way of embedding the glTF export panel here so we don't need to do that manually, I couldn't find an easy way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some checking into embedding the glTF export panel and sadly I don't think it's an option. I don't know if maybe we want to have a toggle between overriding settings and just using the saved glTF properties, but I think if we're going to have overrides then it would be nice to have all the Limit to
options (although that depends on Blender version). Aside from those, the Rest & Ranges
options could affect scene preview, and I think Sampling Animations
should be forced off (have it there, but greyed out and disabled, or maybe just a label that says sample animations disabled?) because it can cause problems with animations and increases the file size.
I'm not really sure what to say about this to be honest. I don't think anything will fit perfectly, and what you have currently, technically, allows advanced users to squeak by and accomplish all possible exports, but it's not super intuitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some limit options and forced:
export_force_sampling: False
use_active_scene: True
as i believe we always want that (we don't support multiple scenes in a glTF anyways)
For the rest I think is fair to rely on the safe export options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is generally good, thanks, but I think the fact we are forcing no sampling of animations should be conveyed to the user, along with our reasoning (in a tooltip I guess), especially since it defaults to on in the regular glTF export. Also, the use_active_scene property was only introduced in Blender 3.2, but yes,having use_active_scene on is useful, but again I think we want to let the user know that because if they do have multiple scenes, but don't know to check that button in the exporter, then I believe it will work for the debugger, but not when they export to upload to Spoke (because they didn't know to check that export option).
@Exairnous I think I've fixed most of the feedback. I've also added a few export flags. Let me know what you think. |
I've added a few more improvements:
Are there any other animation export options worth adding to the overrides panel? |
This reverts commit 3a33ef6.
I think I've addressed all feedback and this is ready for shipping.
Oh yep, I've added that so we always bring to front if not alive.
That must have get lost while migrating the preferences. Re-added.
Yeah, I've done some reorg to better fit all he new fs files. |
Thank you, this is looking excellent!
I'm sorry, it's almost ready for shipping, but not quite. I was finally able to find reproducible steps to trigger the GLB failing to drop in-room:
Aside from this, I think you didn't answer two of my inline comments?
Re export options:
If you generally agree with these two comments, but don't want to spend the time on them I'm happy to implement them in a followup PR or something. Finally, if people use debug local scene and then try to open the room without debug local scene they'll run into this bug: Hubs-Foundation/hubs#6179. This isn't anything to do with this PR, except more people may run into it now, so if you see anyone getting stuck on it, just point them to the GitHub issue for the workaround. |
Thanks again. I fixed all the above.
Fixed
Fixed
Yes, better address it in a follow-up PR. Thanks
I'll take a look and fix this next week before releasing this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, then I think it's good to land (once Jim's issue he reported in Discord, Blender hanging when hitting the close button with Chrome open on Windows, is resolved) and I'll post a followup PR.
Good catch! Also, I've just been testing now and it seems like exporting multiple scenes no longer breaks Hubs, so maybe we don't need to force exporting the active scene now. Can you confirm? |
I've added the followup PR and will update that as needed to any changes here. |
Overall, looks good. Blender already has an open link operator though |
Actually, the room flags having equals signs can cause the update scene vs spawn object check to return the wrong one because it's looking for the exact string of "debugLocalScene" in a list and not finding it. |
Thank you for the updates, but I have some accessibility concerns with the Is Active status indicators being red and green. For example, here is a simulation of Green-Blind/Deuteranopia applied to the indicators (https://www.color-blindness.com/coblis-color-blindness-simulator/) I didn't mention anything with the other status indicators because they were never the only indications of status, but these are. I also think the room flags should have the actual parameter in brackets at the end of the tooltip. As it is, the actual parameters are only referenced in the Python variable name which is only visible if people have turned Python tooltips on (they're off by default) and even then the |
Maybe we can add the text info to the right (On/Off)?
Yeah I guess it wouldn't harm. Let's open PRs/issues for this to track |
Scene debugging is a tedious process and involves:
This PR adds a
Create
andUpdate
buttons to the tools panel to simplify the room creation and the room scene update process.This PR depends on Hubs-Foundation/hubs#6383