Hubs-Foundation / hubs-blender-exporter

The Hubs Blender Add-on enables artists to export their creations to the immersive web with Hubs
Mozilla Public License 2.0
165 stars 51 forks source link

Scene publishing support #255

Closed keianhzo closed 7 months ago

keianhzo commented 9 months ago

This PR adds support for scene publishing

Exairnous commented 9 months ago

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

keianhzo commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

Yep, taht sounds great!