Archipack's call for release code review #52120

Closed
opened 2017-07-19 01:52:27 +02:00 by stephen leger · 13 comments
Member

Hi,
Archipack addon may target huge user base, is stable enougth, and a such great step forward to boost arch productivity to deserve a true code review as ground basis to at least talk about include in release, so here is my request for code review !

As it's a huge and growing project, maybe we should start review of current stable found in addon_contribs (equal to stable branch in github repo) and then the less stable (including roof and materials library handling) on preset_menu branch into github repo.

Overall users / testers / devs feedback currently range between "can't wait for this" , "Fantastic...!!" and "Such a nice day to see this^^"

Features list (available parametric objects)

  • Wall
  • Window
  • Door
  • Floor
  • Slab
  • Truss
  • Fence
  • Stair
  • Upcomming Roof

Road Map

Short term:

  • Roof generator near beta state. Basis is strong, but cutting features should adress some precision issues to be production ready.

Mid term:

  • Add parametric cabinets, possibly kitchen generator, include and enhance must wanted existing objets.
  • Enhance wall mesh creation without solidify limitations.
  • Enhance 2d to 3d workflow

Long term:

  • Gl integration for 2.8

Good news
Found a well established partner to provide a dedicated free default material library through her website.

Release cycle:
Archipack still young and envolving project, release cycle must be shorter than blender's one.
To adress this issue and be able to provide every end user the last stable version, archipack use addon-updater.
Lastest stable versions will be released on regular blender release cycle.

Current issues, and wish list, 2.8 issues.

Working on this addon, found the worst issue is with deps graph and booleans (a fairly known issue), wich prevent eg windows to be direct child of wall, if hole are childs of windows.
From user point of view this is realy anoying, as it enforce use of a "parent of wall and hole" to prevent dep cycle.

Eventing system and operators.
Using snap through regular transform.translate call from other operators require hacks like using draw function as callback to provide real-time user feedback.
Wish to be able to force an operator to let bubble events up (pass_through) while running to handle user feed-back from outside in a cleaner way.
With such option, exposing a snap in python api will even not be necessary.

Events (un)consistancy.
Left select, emulate 3 button mouse does trigger different events for same user action.
From dev point of view it's near nightmare (and nobody else never use it, so..), eg this prevent use of alt as 3 mouse button may intercept and trigger unwanted behaviour.

2.8 current state
I know the gl api for 2.8 still to be done, and currently the addon fails miserabily with an "unknown constant" in bgl.
So wont fix until api is there and somewhat stable.

Python deps:
In order to provide a quick polygon detection without rewriting every single bit, the addon require shapely. This external python dep has proven to be an issue for many users. (allready write intersection detection from scratch to adress precision and performance issues, but building polygons from lines and compute polygon relationships is pretty fast with shapely)

Interoperability:
Most BIM systems currently made use of open-cascade to handle "boundary representations". Current blender implementation of nurbs is realy limited. Using open-cascade may provide interoperability and a full fledged nurbs implementation.
I can live without this one, but may be worth to look at it.

Units setup:
Defaults for values are hardcoded, and match one perticular unit system. Working with "real-world" object, would be great to be able to define the unit system of defaults on addon basis and let properties make changes according when reading default values. A painfull fix envolve unit check and adding multiplier constant on each default, definitely not gracefull way to go.

Archipack network
BA thread
Github repo preset_menu branch
Github Wiki
Yt channel

Hi, Archipack addon may target huge user base, is stable enougth, and a such great step forward to boost arch productivity to deserve a true code review as ground basis to at least talk about include in release, so here is my request for code review ! As it's a huge and growing project, maybe we should start review of current stable found in addon_contribs (equal to stable branch in github repo) and then the less stable (including roof and materials library handling) on preset_menu branch into github repo. Overall users / testers / devs feedback currently range between "can't wait for this" , "Fantastic...!!" and "Such a nice day to see this^^" **Features list** (available parametric objects) - Wall - Window - Door - Floor - Slab - Truss - Fence - Stair - Upcomming Roof **Road Map** Short term: - Roof generator near beta state. Basis is strong, but cutting features should adress some precision issues to be production ready. Mid term: - Add parametric cabinets, possibly kitchen generator, include and enhance must wanted existing objets. - Enhance wall mesh creation without solidify limitations. - Enhance 2d to 3d workflow Long term: - Gl integration for 2.8 **Good news** Found a well established partner to provide a dedicated free default material library through her website. **Release cycle:** Archipack still young and envolving project, release cycle must be shorter than blender's one. To adress this issue and be able to provide every end user the last stable version, archipack use addon-updater. Lastest stable versions will be released on regular blender release cycle. **Current issues, and wish list, 2.8 issues.** Working on this addon, found the worst issue is with deps graph and booleans (a fairly known issue), wich prevent eg windows to be direct child of wall, if hole are childs of windows. From user point of view this is realy anoying, as it enforce use of a "parent of wall and hole" to prevent dep cycle. Eventing system and operators. Using snap through regular transform.translate call from other operators require hacks like using draw function as callback to provide real-time user feedback. Wish to be able to force an operator to let bubble events up (pass_through) while running to handle user feed-back from outside in a cleaner way. With such option, exposing a snap in python api will even not be necessary. Events (un)consistancy. Left select, emulate 3 button mouse does trigger different events for same user action. From dev point of view it's near nightmare (and nobody else never use it, so..), eg this prevent use of alt as 3 mouse button may intercept and trigger unwanted behaviour. 2.8 current state I know the gl api for 2.8 still to be done, and currently the addon fails miserabily with an "unknown constant" in bgl. So wont fix until api is there and somewhat stable. Python deps: In order to provide a quick polygon detection without rewriting every single bit, the addon require shapely. This external python dep has proven to be an issue for many users. (allready write intersection detection from scratch to adress precision and performance issues, but building polygons from lines and compute polygon relationships is pretty fast with shapely) Interoperability: Most BIM systems currently made use of open-cascade to handle "boundary representations". Current blender implementation of nurbs is realy limited. Using open-cascade may provide interoperability and a full fledged nurbs implementation. I can live without this one, but may be worth to look at it. Units setup: Defaults for values are hardcoded, and match one perticular unit system. Working with "real-world" object, would be great to be able to define the unit system of defaults on addon basis and let properties make changes according when reading default values. A painfull fix envolve unit check and adding multiplier constant on each default, definitely not gracefull way to go. **Archipack network** [BA thread](https://blenderartists.org/forum/showthread.php?417856-Addon-Archipack) [Github repo preset_menu branch](https://github.com/s-leger/archipack/tree/preset_menu) [Github Wiki](https://github.com/s-leger/archipack/wiki) [Yt channel](https://www.youtube.com/channel/UCecdw78pgsch0FWaNhdwbgg)
Author
Member

Changed status to: 'Open'

Changed status to: 'Open'
Author
Member

Added subscriber: @StephenLeger

Added subscriber: @StephenLeger
Member

Added subscribers: @mont29, @dr.sybren, @BrendonMurphy

Added subscribers: @mont29, @dr.sybren, @BrendonMurphy
Member

hi, i think we could raise an exception for this addon and release with 2.79
Given the addon is of a good quality and actively developed, with the author also active in irc and helping others.
I would be happy to accept this as last addition to addons for 2.79.
My only concern here is the updater.
The updater is based on the cgcookie updater, I have seen a couple of addons adopting this method.
https://github.com/CGCookie/blender-addon-updater
i will ask opinion of @mont29 and @dr.sybren on both the updater (keep to allow users to update the ongoing work after 2.79 or remove if the updater is deemed invalid method) and on the exemption to add to release at this very late stage.
Archipack is a forward thinking addon with new & usable methods for architectural modelling, well designed & tested, I think it's worth to supply users with early access whilst it's still in great shape in 2.79 api
Thanks

hi, i think we could raise an exception for this addon and release with 2.79 Given the addon is of a good quality and actively developed, with the author also active in irc and helping others. I would be happy to accept this as last addition to addons for 2.79. My only concern here is the updater. The updater is based on the cgcookie updater, I have seen a couple of addons adopting this method. https://github.com/CGCookie/blender-addon-updater i will ask opinion of @mont29 and @dr.sybren on both the updater (keep to allow users to update the ongoing work after 2.79 or remove if the updater is deemed invalid method) and on the exemption to add to release at this very late stage. Archipack is a forward thinking addon with new & usable methods for architectural modelling, well designed & tested, I think it's worth to supply users with early access whilst it's still in great shape in 2.79 api Thanks
Brendon Murphy self-assigned this 2017-07-19 10:10:17 +02:00

Added subscriber: @ideasman42

Added subscriber: @ideasman42

Imho, if archipack wants to go in Blender-released addons, it should get rid of things like updater - we don’t want third party package-management-like things in our distributed addons…

Regarding code itself, well, actually reviewing it would take ages, but from very quick look it seems reasonably clean.

@ideasman42 may also have some things to say here?

Imho, if archipack wants to go in Blender-released addons, it should get rid of things like updater - we don’t want third party package-management-like things in our distributed addons… Regarding code itself, well, actually reviewing it would take ages, but from very quick look it seems reasonably clean. @ideasman42 may also have some things to say here?
Author
Member

Hi,
Removed addon-updater and commit 1.2.6 into addon_contribs.

Hi, Removed addon-updater and commit 1.2.6 into addon_contribs.
Member

Added subscriber: @blend-it

Added subscriber: @blend-it
Member

@StephenLeger if no-one raises an objection you can commit in 2 days.
After some discussions in irc, i don't see any problem with this.
Thanks

@StephenLeger if no-one raises an objection you can commit in 2 days. After some discussions in irc, i don't see any problem with this. Thanks
Member

hi, just noticed some context issues with the panel, it's easy fixed by setting the panel to object mode only

hi, just noticed some context issues with the panel, it's easy fixed by setting the panel to object mode only
Member

hi, this is ready to land, thanks for quick fix.

hi, this is ready to land, thanks for quick fix.
Member

Changed status from 'Open' to: 'Resolved'

Changed status from 'Open' to: 'Resolved'
Member

@StephenLeger thanks and welcome, congrats.
Closing as resolved

@StephenLeger thanks and welcome, congrats. Closing as resolved
Sign in to join this conversation.
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender-addons#52120
No description provided.