brunopostle / homemaker-addon

Design buildings the pointy-clicky way, a blender add-on that transforms spatial geometry into IFC building models
GNU General Public License v3.0
125 stars 7 forks source link

Switch to sub-classing Topologic #32

Closed brunopostle closed 1 year ago

brunopostle commented 2 years ago

The Homemaker add-on ships with a module called topologist that provides additional methods that are not provided by the various topologic classes.

For instance, import topologist.face appends a new topologic.Face.IsVertical() method, which is unexpected if you think that Topologic should have its own namespace.

This behaviour made sense initially due to the way the Topologic C++ bindings were implemented, but it no longer makes any sense, Homemaker should subclass topologic as a normal well-behaved library should.

Classes that are overloaded like this are Cell, CellComplex, Edge, Face, Graph, Topology, and Vertex.

brunopostle commented 1 year ago

Further, Homemaker should switch to using the @wassimj / topologicpy API for Topologic. There are probably also a bunch of methods in the topologist module that should be moved out of Homemaker and into topologicpy

wassimj commented 1 year ago

@brunopostle if you can send me a list of methods and the topologicpy class that they should belong to (e.g. Face, Cell, CellComplex), I can check them, refactor them to fit the coding style of topologicpy and start adding as appropriate. Thank you.

brunopostle commented 1 year ago

@wassimj The methods I add to topologic are listed here, though I suspect there are only a handful that are generically useful: https://homemaker-addon.readthedocs.io/en/latest/api-topologist.html

wassimj commented 1 year ago

@brunopostle thank you. Some of the methods have already been incorporated in topologicpy. Some, as you indicated, are not suitable for a general API, but will see what can be integrated. Thanks again.

brunopostle commented 1 year ago

I made another failed attempt to subclass the topologic python bindings, either I don't understand what I'm doing or the bindings are configured in a way that precludes subclassing.

This demonstrates my problem, basically constructors such as ByStartVertexEndVertex() return objects of the super class, not the subclass:


import topologic

class MyEdge(topologic.Edge):
    def __init__(self):
        return

foo = MyEdge

print(foo)
# <class '__main__.MyEdge'>

bar = MyEdge.ByStartVertexEndVertex(
    topologic.Vertex.ByCoordinates(0.4, -2.2, 0.3),
    topologic.Vertex.ByCoordinates(0.5, -2.4, 0.3),
)

print(bar)
# <topologic.Edge object at 0x7fb385ed6d30>
wassimj commented 1 year ago

You didn't override the parent method which doesn't know anything about your class so it returns whatever it always returns: a topologic.Edge. In topologicpy, I decided not to subclass from topologic. Instead it is a wrapper class that accepts and returns topologic entities and adds new methods.

Some possible ways you can subclass are listed at: https://stackoverflow.com/questions/597199/converting-an-object-into-a-subclass-in-python

Athro Wassim Jabi, Ph.D. (fe, e, ef) Cadeirydd Dulliau Cyfrifiadurol ym maes Pensaernïaeth Ysgol Pensaernïaeth Cymru

Prifysgol Caerdydd

Adeilad Bute

Rhodfa’r Brenin Edward VII

Caerdydd CF10 3NB

Cymru D.U. Ffôn : +44 (0)29 2087 5981 Ebost: @.**@.>

Professor Wassim Jabi, Ph.D. (he, his, him) Chair of Computational Methods in Architecture Welsh School of Architecture

Cardiff University

Bute Building King Edward VII Avenue Cardiff CF10 3NB Wales U.K. Phone: +44 (0)29 2087 5981 Email: @.**@.>

Mae Prifysgol Caerdydd yn elusen gofrestredig. Rhif 1136855 Cardiff University is a registered charity. No 1136855

Yn falch o fod yn un o’r 3 Ysgol Pensaernïaeth orau yn y DU |The Times/The Sunday Times Good

University Guide 2023 | 4ydd yn y DU: REF 2021| Canolfan ragoriaeth sy'n arwain y byd ym meysydd pensaernïaeth a'r amgylchedd adeiledig

Prifysgol y Flwyddyn yng Nghymru 2023

Proud to be a top 3 UK School of Architecture |The Times/The Sunday Times Good University Guide 2023 | 4th in the UK: REF 2021| A world-leadingcentre of excellence for architecture

and the built environment

Welsh University of the Year 2023


From: Bruno Postle @.> Sent: Wednesday, February 8, 2023 11:48:44 PM To: brunopostle/homemaker-addon @.> Cc: Wassim Jabi @.>; Mention @.> Subject: Re: [brunopostle/homemaker-addon] Switch to sub-classing Topologic (Issue #32)

External email to Cardiff University - Take care when replying/opening attachments or links. Nid ebost mewnol o Brifysgol Caerdydd yw hwn - Cymerwch ofal wrth ateb/agor atodiadau neu ddolenni.

I made another failed attempt to subclass the topologic python bindings, either I don't understand what I'm doing or the bindings are configured in a way that precludes subclassing.

This demonstrates my problem, basically constructors such as ByStartVertexEndVertex() return objects of the super class, not the subclass:

import topologic

class MyEdge(topologic.Edge): def init(self): return

foo = MyEdge

print(foo)

<class 'main.MyEdge'>

bar = MyEdge.ByStartVertexEndVertex( topologic.Vertex.ByCoordinates(0.4, -2.2, 0.3), topologic.Vertex.ByCoordinates(0.5, -2.4, 0.3), )

print(bar)

<topologic.Edge object at 0x7fb385ed6d30>

— Reply to this email directly, view it on GitHubhttps://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fbrunopostle%2Fhomemaker-addon%2Fissues%2F32%23issuecomment-1423387171&data=05%7C01%7Cjabiw%40cardiff.ac.uk%7Cfad7b79596f1473ff97408db0a2f044d%7Cbdb74b3095684856bdbf06759778fcbc%7C1%7C0%7C638114969268445803%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=uWhRg7LT5zQnPUCcE3qBy31OhS1pJVoqrTy9ZdWoG6E%3D&reserved=0, or unsubscribehttps://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABQCZPOOPQW7QCUL22VM7GTWWQWFZANCNFSM5JMP7X2Q&data=05%7C01%7Cjabiw%40cardiff.ac.uk%7Cfad7b79596f1473ff97408db0a2f044d%7Cbdb74b3095684856bdbf06759778fcbc%7C1%7C0%7C638114969268445803%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=JzMn4OYlCuCD68S4mUKLFu325V0MSYqMOQsInw2fQnk%3D&reserved=0. You are receiving this because you were mentioned.Message ID: @.***>

brunopostle commented 1 year ago

Hmm, I'm not keen on writing wrappers for every topologic method that returns topologic objects.

I'm currently doing 'python monkey patching', which works very well, but it does mean that users will see all my custom methods in the topologic namespace. I guess I shouldn't worry, this would only be a problem if a conflicting method name was added to the base library.

Closing, because migrating to topologicpy is another thing altogether.

wassimj commented 1 year ago

ok. But just to note that topologicpy is the future of how 3rd party python modules will interact with topologic.