Maniphest T69936

Update sun_position to Blender 2.82
Closed, ResolvedPATCH
Actions

Assigned To
Brendon Murphy (meta-androcto)
Authored By
Damien Picard (pioverfour)
Sep 16 2019, 11:33 AM
Tags
  • Add-ons (Community)
Subscribers
Brendon Murphy (meta-androcto)
Damien Picard (pioverfour)
nBurn (nBurn)
Tokens
"Love" token, awarded by ugosantana.

Description

Hi, I see that @Brendon Murphy (meta-androcto) updated the sun_position add-on in rBAC7b0831a77528. I have been working on updating it as well for a while, and although the map feature is still missing, the rest of the add-on is done, and is much better integrated into Blender’s UI/UX.

Here is a list of some changes:

  • Move preferences to user preferences
  • Convert UI to flow, remove two-column mode
  • Remove activation operator. The add-on is now always on
    • Update is done with prop update functions and a handler instead
  • Disabling options in preferences now have an effect on the scene
  • HDR selection is now an simpler operator. Click in 3D view to select sun position
  • Presets are now actual Blender presets, so the user can add their own
  • Object groups for analemma visualisation is now a collection
  • Cleanup and refactor
    • Remove some manager classes and operators
    • Style (some pep8, snake_case, variable names)

I rebased upon rBAC7b0831a77528, so the patch should apply cleanly. I won’t push this straight away as it’s not a small change.

sun_pos_update_2.8.patch154 KBDownload

Related Objects

Mentioned In
rBACeb3e843c232a: sun_position: remove from contrib: T69936
rBAefbc5e5db7c7: sun_position: move to release: T69936
T71560: Blender 2.82 New Addons Proposals
rBACca85dbc76c57: sun position: update working version: T69936
D5825: Update sun_position to Blender 2.8
Mentioned Here
rBACbd4f0a0c1f30: sun_position: fix rotation order
rBACab0c2572c13b: sun_position: remove world map operator and image file
rBAC6eb516cda1f8: sun_position: update presets
rBACc74a4e7d5ae2: sun_position: set default date to today
rBACb7aae316792d: sun_position: cleanup properties
rBACe79295c648af: sun_position: fix time calculations
rBACe8696b04ee43: sun_position: UTC zone as float
rBACb52e7760ff6c: sun_position: fix errors when selecting wrong types of node
D5825: Update sun_position to Blender 2.8
rBAC7b0831a77528: sun position: base update

Event Timeline

Damien Picard (pioverfour) lowered the priority of this task from 90 to Normal.Sep 16 2019, 11:33 AM
Damien Picard (pioverfour) created this task.
Damien Picard (pioverfour) updated the task description.Sep 16 2019, 11:41 AM
Brendon Murphy (meta-androcto) added a comment.Sep 17 2019, 5:15 AM

hi, nice work. we can do this I'll check with you in blender.chat before go ahead. nBurn and I were discussing some issues a couple of days ago in irc, it will be worth a quick discuss before push.

Damien Picard (pioverfour) edited a custom field.Sep 17 2019, 11:48 AM
Damien Picard (pioverfour) added a comment.Sep 17 2019, 12:25 PM

That’s fine with me. Some points which certainly need discussion are:

  • The reimplementation of the HDR and world map modes, which were in my opinion overly complex in the previous version, but maybe they fit others’ use cases. The shader part can definitely be simplified using the new gpu and gpu_extras modules, but I’m not sure about the UI.
    • For the HDR sun selection, I don’t think the user needs to display it permanently, since the purpose is to choose a precise point on the texture and then sync it, so a one-time operation fits better.
    • For the world map, I’m not sure overlaying it on the 3D View or properties panel is right. Maybe displaying it inside an image editor would be better, but I’m not sure yet.
  • The presets: I made a non-standard operator to copy defaults to the user directory, but I’m not sure it’s really useful for everybody (two Boston presets, two Honolulu presets…?).
  • The UTC zone’s behaviour is not clear to me, despite its description in the old wiki.

I have only tested the add-on under Linux, so there might be problems with graphics on other platforms.

Should I open a Diff to further review the code?

Damien Picard (pioverfour) mentioned this in D5825: Update sun_position to Blender 2.8.Sep 17 2019, 4:43 PM

I opened a diff at D5825, and restored mirrorball projections for HDRI sun selections.

Brendon Murphy (meta-androcto) added a subscriber: nBurn (nBurn).Sep 17 2019, 11:41 PM

hi @Damien Picard (pioverfour)

The re implementation of the HDR and world map modes, which were in my opinion overly complex in the previous version, but maybe they fit others’ use cases. The shader part can definitely be simplified using the new gpu and gpu_extras modules, but I’m not sure about the UI.

  • I would not worry about the hrdi/world map just yet.
  • The hdri button can stay for now if it's working.
  • The World map, keep the current code intact, it's ok for now to leave it. There's several options on how to handle it,
    • Implement in old style.
    • Put in some kind of open/close show/hide panel.
    • Possibly there's code for custom editor types now, could even do that and pop it up?

We do need more thinking on that and it's not a priority atm. I think it's best to wait, I've asked newbs to look in a month when he's back from holiday. There's other bgl experts that may be able to help/provide answers yet to be contacted.

  • The presets become a little more important if there's no world map? I think at some later stage they could be improved.

Some history with this fix:
https://github.com/kevancress/sun_position_b28_test was the base for the update.
@nBurn (nBurn) had a look and made some changes, then I made some changes.
Now it's in your hands so to speak. I guess at one stage there were 3 of us working on it...

Your code has had basic look over, it looks ok, much cleaner than what we initially committed. Thanks! If you can could you zip upload the whole file here in this task. I want to look at it as a whole addon and I'll compare all the files, double check some issues I was looking at.
Thanks again.

Damien Picard (pioverfour) added a comment.Sep 18 2019, 11:00 AM

All right, I’ll give it a rest for the time being, the map can be reimplemented later. The current code for it won’t work at all since it relies on the Display manager class, which I removed. But it was broken for a long time anyway, I had to use Blender 2.75 to see how it behaved.

Thanks for the history, I didn’t know so much work had gone into this already…

Anyway, here is the zipped add-on.

190918_sun_position.zip122 KBDownload

Damien Picard (pioverfour) added a comment.Sep 18 2019, 6:15 PM

Some fixes for the HDRI point selector, implemented geographic coordinates parser field using a GPL module by Maximilian Högner, to allow users to paste coordinates from an online map.

190918_sun_position.v01.zip124 KBDownload

Brendon Murphy (meta-androcto) mentioned this in rBACca85dbc76c57: sun position: update working version: T69936.Sep 20 2019, 8:24 AM
Brendon Murphy (meta-androcto) mentioned this in T71560: Blender 2.82 New Addons Proposals.Nov 14 2019, 2:46 AM
Ugo Santana (ugosantana) awarded a token.Nov 17 2019, 12:23 AM
Brendon Murphy (meta-androcto) triaged this task as 50 priority.Dec 3 2019, 1:43 AM

hi, still some work to do here. After discussions we will remove the not working world map and restore some sane presets.
Small code clean up will follow and committed to contrib.
We will then look at viability of 2.82 release.

Brendon Murphy (meta-androcto) renamed this task from Update sun_position to Blender 2.8 to Update sun_position to Blender 2.82.Dec 3 2019, 1:44 AM
Damien Picard (pioverfour) added a comment.Dec 4 2019, 12:10 PM

I proceeded with removing the world maps (rBACab0c257), and replacing presets (rBAC6eb516c).
I also did various fixes and cleanups (rBACb7aae31, rBACbd4f0a0, rBACc74a4e7, rBACb52e776) and fixes to match the online calculator upon which this add-on is based (rBACe79295c, rBACe8696b0).

Brendon Murphy (meta-androcto) added a comment.Dec 9 2019, 2:13 AM

hi, thanks for all the requested changes. This is now ready to land.
Please tag this task in the 2 commits. 1st remove from addons contrib, 2nd commit to add to release addons.

Damien Picard (pioverfour) mentioned this in rBAefbc5e5db7c7: sun_position: move to release: T69936.Dec 9 2019, 12:05 PM
Damien Picard (pioverfour) mentioned this in rBACeb3e843c232a: sun_position: remove from contrib: T69936.
Brendon Murphy (meta-androcto) changed the task status from Unknown Status to Resolved.Dec 9 2019, 10:43 PM

committed and closing. Thanks @Damien Picard (pioverfour) for the time and effort put into this.