Scene publishing support #255

keianhzo · 2023-12-13T14:55:49Z

This PR adds support for scene publishing

Exairnous · 2023-12-19T05:44:09Z

Thanks! I'll review this as soon as I can.

Exairnous

My apologies for the long delay in reviewing this. I think this will be a fantastic addition, but I have some notes:

The attribution field and remixing/promotion checkboxes are missing.
The json module is needed several times in the api.py file, so I think it might as well be imported at the top of the file.
Why were the context pointers, etc. changed in environment_settings.py and operators.py? They seem unrelated.
I talked to @j-conrad at one point and he mentioned that he had added the old update scene operator to the quick favorites menu and now it updates the published scene rather than the local scene in the room. I know this is because the idnames were updated to be more descriptive, but it might be nice if they could be tweaked slightly so this could be avoided.
The warning for needing to be signed in should only be applied when updating the scene, not spawning objects, also it looks like my fix for allowing spawned objects hasn't been merged in (although, my fix may need to be tweaked because of the new requirements of sign in for publishing/updating scenes).
I'm not sure the annotations you added are needed. Currently they're applied inconsistently and they don't provide any additional information in my opinion.
I don't think the scene list should be cached and/or saved to the json prefs. This adds a lot of complexity just to allow a speed boost in getting scenes for an instance when reloaded, can easily be outdated, and can cause poll issues with the other operators in that section when switching instances (because the items are technically still there, just hidden). Unless it's really slow getting a long list of scenes (once the unneeded export scene is removed), I don't think the caching is worth it. Also, I think the scene list should be cleared when the session is closed.
Underscores shouldn't be allowed in the scene name and a warning should be displayed. A warning should also be shown for an empty scene name.
I don't think there's any way to update the screenshot/room name/attribution/remixing/promotion of an already published scene. It would be nice if there were a checkbox or something to allow this.
The update scene confirmation doesn't provide very good user feedback in Blender 4.0 (the menu doesn't disappear after clicking on yes for me). I'm not sure if there's anything to do about this or not, but I thought I should mention it.

addons/io_hubs_addon/api.py

addons/io_hubs_addon/debugger.py

Exairnous · 2024-02-02T13:59:56Z

addons/io_hubs_addon/debugger.py

+            screenshot = context.scene.hubs_scene_debugger_scene_publish_props.screenshot
+            screenshot_full = bpy.path.abspath(
+                screenshot.filepath, library=screenshot.library)
+            screenshot_norm = os.path.normpath(screenshot_full)


I don't think this has been needed for paths elsewhere, what's the reason for adding it here?

Honestly I don't remember, I might have run into some issues at the time. I don't think it harms.

Not really, I think I've might read about it at the time.

addons/io_hubs_addon/debugger.py

Exairnous · 2024-02-02T14:05:45Z

addons/io_hubs_addon/debugger.py

+        name="Id",
+        description="Scene id",
+        default="Scene id",
+        get=lambda self: self["scene_id"]


Why were lambdas added for the get methods of the properties in this property group only?

I think it was simple enough to be a good lambda case.

But doesn't it just do the same as the default? If so, why add it?

keianhzo · 2024-02-13T10:51:57Z

Thanks a lot for the review. I think I have addressed quite some important issues. for the rest:

The attribution field and remixing/promotion checkboxes are missing.

Yes, that's important, but doing it right will require some time and I would rather not be delaying this feature anymore as I think it can be very useful. Maybe some good Samaritan would be willing to add support for that. It shouldn't be difficult, it's mostly a matter of adding a list with "creator" and "url" that get added to the attributions object in scene_data when publishing or updating. We would probably need two lists UI widgets, one inside "manage" and another one inside "Publish".

Why were the context pointers, etc. changed in environment_settings.py and operators.py? They seem unrelated.

We are using the hubs_open_image operator for the screenshot, and I needed to decouple it from components and use getattr/setattr to make it work with context.scene.

I talked to @j-conrad at one point and he mentioned that he had added the old update scene operator to the quick favorites menu and now it updates the published scene rather than the local scene in the room. I know this is because the idnames were updated to be more descriptive, but it might be nice if they could be tweaked slightly so this could be avoided.

This shouldn't happen anymore, right? I updated the name as you described above, hope that helps.

I don't think the scene list should be cached and/or saved to the json prefs. This adds a lot of complexity just to allow a speed boost in getting scenes for an instance when reloaded, can easily be outdated, and can cause poll issues with the other operators in that section when switching instances (because the items are technically still there, just hidden). Unless it's really slow getting a long list of scenes (once the unneeded export scene is removed), I don't think the caching is worth it. Also, I think the scene list should be cleared when the session is closed.

We can address this in a follow-up if it's a real problem, dealing with undoing that delay this release quite a lot.

Underscores shouldn't be allowed in the scene name and a warning should be displayed. A warning should also be shown for an empty scene name.

Another good follow-up.

I don't think there's any way to update the screenshot/room name/attribution/remixing/promotion of an already published scene. It would be nice if there were a checkbox or something to allow this.

Related to my comment above.

The update scene confirmation doesn't provide very good user feedback in Blender 4.0 (the menu doesn't disappear after clicking on yes for me). I'm not sure if there's anything to do about this or not, but I thought I should mention it.

Same, good follow up.

Exairnous · 2024-02-13T12:09:19Z

Thanks a lot for the review.

You're welcome :)

We are using the hubs_open_image operator for the screenshot, and I needed to decouple it from components and use getattr/setattr to make it work with context.scene.

Ah, okay. Thanks.

This shouldn't happen anymore, right? I updated the name as you described above, hope that helps.

This was an issue with the idname for the new HubsUpdateSceneOperator being the same as the old idname for the HubsUpdateRoomOperator. I don't think you changed anything there, so if people had the old operator bound to the quick favorites menu it's probably still triggering the new operator now. But it was only a small issue, I doubt many people will run into it and Jim already knows to re-add it.

We can address this in a follow-up if it's a real problem, dealing with undoing that delay this release quite a lot.

Well, it introduces a number of corner cases, but sure, it can be updated in a followup PR.

I guess, if it's alright with you, I'll address the remaining things in followup PRs after the release.

keianhzo · 2024-02-13T14:45:06Z

Yep, taht sounds great!

keianhzo force-pushed the scene-publishing branch from da41d35 to a30a52e Compare December 13, 2023 15:08

keianhzo marked this pull request as draft December 14, 2023 09:43

keianhzo force-pushed the scene-publishing branch 2 times, most recently from 3d0a6df to 5e0de04 Compare December 15, 2023 14:14

keianhzo added this to the 1.3.1 milestone Dec 18, 2023

keianhzo added 6 commits December 18, 2023 15:58

Scene publishing support

d7467c6

Fix lint

da5270b

Rebase fixes

ad3523a

Fix reticulumUrl param

4b333db

Linter

ed61cd9

Fix local room with scene creation

a5a2d73

keianhzo force-pushed the scene-publishing branch from 5e0de04 to a5a2d73 Compare December 18, 2023 14:59

keianhzo marked this pull request as ready for review December 18, 2023 15:00

keianhzo requested a review from Exairnous December 18, 2023 15:00

keianhzo mentioned this pull request Dec 19, 2023

Expose reticulum url Hubs-Foundation/hubs#6424

Merged

Exairnous requested changes Feb 2, 2024

View reviewed changes

keianhzo added 5 commits February 13, 2024 10:39

Remove duplicated line

a291e77

Fix overwritten imports

b32e797

Update message

f6a5e5d

Add in memory screenshot support

ad39d4e

Address other feedback

e2d3831

keianhzo merged commit 30c4b26 into master Feb 13, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scene publishing support #255

Scene publishing support #255

keianhzo commented Dec 13, 2023

Exairnous commented Dec 19, 2023

Exairnous left a comment

Exairnous Feb 2, 2024

keianhzo Feb 13, 2024

keianhzo Feb 13, 2024

Exairnous Feb 2, 2024

keianhzo Feb 13, 2024

Exairnous Feb 13, 2024

keianhzo commented Feb 13, 2024 •

edited

Loading

Exairnous commented Feb 13, 2024

keianhzo commented Feb 13, 2024

Scene publishing support #255

Scene publishing support #255

Conversation

keianhzo commented Dec 13, 2023

Exairnous commented Dec 19, 2023

Exairnous left a comment

Choose a reason for hiding this comment

Exairnous Feb 2, 2024

Choose a reason for hiding this comment

keianhzo Feb 13, 2024

Choose a reason for hiding this comment

keianhzo Feb 13, 2024

Choose a reason for hiding this comment

Exairnous Feb 2, 2024

Choose a reason for hiding this comment

keianhzo Feb 13, 2024

Choose a reason for hiding this comment

Exairnous Feb 13, 2024

Choose a reason for hiding this comment

keianhzo commented Feb 13, 2024 • edited Loading

Exairnous commented Feb 13, 2024

keianhzo commented Feb 13, 2024

keianhzo commented Feb 13, 2024 •

edited

Loading