Delgine 3D Tools & Content DeleD Community Edition
Forums
 
 FAQFAQ   SearchSearch   MemberlistMemberlist   UsergroupsUsergroups   RegisterRegister 
 ProfileProfile   Log in to check your private messagesLog in to check your private messages   Log inLog in 

The geometry structure

 
This forum is locked: you cannot post, reply to, or edit topics.   This topic is locked: you cannot edit posts or make replies.    DeleD Community Edition Forum Index -> DeleD Development
View previous topic :: View next topic  
Author Message
chronozphere
DeleD PRO user


Joined: 20 Jun 2006
Posts: 1010
Location: Netherlands

PostPosted: Fri Jan 22, 2010 9:14 pm    Post subject: The geometry structure Reply with quote

Hey guys

I was trying to fix one of the bugs on the tracker, and I stumbled upon the TBasePrimitive class. I was kinda shocked to see how big it actually was. Shocked I was also trying to make myself familiar with the way "merge" works. That wasn't too easy either.

The geometry part of DeleD is quite complex and needs documentation.
The fact that there are two ways to represent geometry actually worries me (I'm talking about unit_Types.pas v.s unit_Generics.pas). Also I see a that a lot of "extra stuff" is included in the classes. For example:

> Information about the selection
> Cached data
> Temporary variables
> Even GUI related things (TTreeNode in TBasePrimitive)

I understand that it is good to have some of these things locally available. On the other hand, I don't really like working with these "monster classes".

To summarize, I see a great deal of complexity here, and I feel things need to be broken down into smaller pieces that each have a clear task.
Also, I have the feeling that there is quite some redundancy in this part of the source. Confused

@PJ and Jeroen: You have worked on DeleD for about 5 years, so you probably know what's best. What are your views on this? Would it be possible to refactor the geometry code? How to make these things easier to understand and use?

This might just me being an impatient perfectionist, but still...

Anyone?
Back to top
View user's profile Send private message
Jeroen
Site Admin


Joined: 07 Aug 2004
Posts: 5332
Location: The Netherlands

PostPosted: Fri Jan 22, 2010 9:40 pm    Post subject: Reply with quote

Quick answer: don't worry too much about the generics in unit_Generics.pas unless you want to mess with the CSG routines. I think other areas need attention first.

The BasePrimitive is, indeed, very large. I'll get back on that a.s.a.p. Do feel free to elaborate on the redundancy you think you see so I can focus on that when explaining things.

Btw, I worked on DeleD for about 8 years. The first 3 years were experimental (I had to learn OO programming) and I started over several times. PJ and I joined forces in 2004 and in 2005 Delgine was born. Wink
_________________
Check out Figuro, our online 3D app! More powerful 3D tools for free.
Back to top
View user's profile Send private message Send e-mail Visit poster's website
chronozphere
DeleD PRO user


Joined: 20 Jun 2006
Posts: 1010
Location: Netherlands

PostPosted: Fri Jan 22, 2010 11:24 pm    Post subject: Reply with quote

Quote:

Btw, I worked on DeleD for about 8 years. The first 3 years were experimental (I had to learn OO programming) and I started over several times. PJ and I joined forces in 2004 and in 2005 Delgine was born. Wink


Ah.. Good to know about the history. Smile

Ok, I feel these SearchOffsets are redundant. They should not be part of this class. BTW: Most of them don't even seem to be used by the class. They are just sitting there, waiting to be used through their properties.
I'm talking about these:

Code:

  TBasePrimitive = class(TObject)
  private
    fVertexSearchOffset: integer;
    fPolygonSearchOffset: integer;
    fPolygonSearchIndex: integer;
    fEdgeSearchOffset: integer;


If a routine requires a special kind of "state" between executions, I would make a little datastructure for this and pass it as var parameter to the functions who use it. Take a look at FindFirst/FindNext to see what I mean.

Code:

    fName: string;
    fComments: string;
    fTag: string;
    fID: integer;
    fObjectType: TObjectType;
    fTreeNode: TTreeNode;
    fGroupID: integer;
    fOldGroupID: integer;

    fEditContext: TEditContext;
    fCenterSelectedVertices: TVertice;
    fCenter: TVertice;
    fPivot: TVertice;
    fVisible: boolean;
    fFrozen: boolean;
    fAutoUV: boolean;
    fSnapMethod: TSnapMethod;
    fRatio: boolean;


The fields above "place" the geometry into the actual scene. They tell us things about the context of the geometry (how it should be used, edited, shown etc).
These could be separated from the actual geometry code. This would drasticly simplify our class.

Code:

    procedure Polygon_Select(p: TVertice); overload;
    function Polygon_Select(const aRay: TRay): TPolygon; overload;
    procedure Polygon_Select(const aUVCoordinate: TUVCoordinate; const aMaterial: TMaterial); overload;
    function Polygon_Select(const aRay: TRay; out aDistance: single; out aPoly: TPolygon): boolean; overload;
    procedure Polygon_SelectOnSwitchToVertexMode;
    procedure Polygon_SelectOnSwitchToEdgeMode;
    procedure Polygon_SelectAll(const aMaterial: TMaterial = nil);
    procedure Polygon_SelectMultiple(v1,v2: TVertice); overload;
    procedure Polygon_SelectMultiple(p1, p2: TUVCoordinate; const aMaterial: TMaterial); overload;
    procedure Polygon_SelectSystemPolygons;
    procedure Polygon_DeselectAll;
    procedure Polygon_ReverseSelection;
    procedure Polygon_GrowSelection;
    procedure Polygon_ShrinkSelection;

    function Edge_IsSelected(AnEdge: TEdge): boolean;
    function Edge_IsHighLighted(AnEdge: TEdge): boolean;
    procedure Edge_SelectAll;
    procedure Edge_SelectMultiple(v1,v2: TVertice);
    procedure Edge_Select(p: TVertice; range: double); overload;
    function Edge_SelectByPoint(p: TVertice; range: double): TEdge;
    procedure Edge_Select(AEdge: TEdge); overload;
    procedure Edge_Select(aRay: TRay; aPolygon: TPolygon; aDistance: single; aAddToSelection: boolean); overload;
    procedure Edge_SelectOnSwitchToVertexMode;
    procedure Edge_SelectOnSwitchToPolygonMode;
    procedure Edge_SelectLoop;
    procedure Edge_SelectRing;
    procedure Edge_ReverseSelection;
    procedure Edge_GrowSelection;
    procedure Edge_ShrinkSelection;
    procedure Edge_DeselectAll;
    procedure Edge_Highlight(p: TVertice; range: double);
    procedure Edge_DehighlightAll;

    function Vertex_IsSelected(AVertex: TVertice): boolean;
    function Vertex_IsHighlighted(AVertex: TVertice): boolean;
    procedure Vertex_SelectAll;
    procedure Vertex_SelectMultiple(v1,v2: TVertice);
    procedure Vertex_GetSelected(AList: TVerticeList);
    procedure Vertex_Select(p: TVertice; range: double); overload;
    function Vertex_SelectByPoint(p: TVertice; range: double): TVertice; overload;
    procedure Vertex_Select(AVertex: TVertice); overload;
    procedure Vertex_SelectOnSwitchToEdgeMode;
    procedure Vertex_SelectOnSwitchToPolygonMode;
    procedure Vertex_ReverseSelection;
    procedure Vertex_GrowSelection;
    procedure Vertex_ShrinkSelection;
    procedure Vertex_DeselectAll;
    procedure Vertex_Highlight(p: TVertice; range: double);
    procedure Vertex_DehighlightAll;
    procedure Vertex_SelectedPolygonsToSelectedVertices;
    procedure Vertex_SelectedEdgesToSelectedVertices;


The whole "selection" aspect could be separated aswell. As you see, these routines could be even separated into multiple classes. Smile

Okay, I'll now quickly describe an alternate design, by going through all the aspects of the current TBasePrimitive.

> The geometry: This is the actual data of the primitive. There are a lot of functions that operate on this data. This functionality should be stored in it's own class. I call it "TPrimitiveGeometry".

> The scene information: This is the information of a primitive in a scene. It will contain name, ID, tag, groupID etc. I call this class "TPrimitiveInfo".

> The selection: The selection aspect is quite different. It consists of a lot of routines operating on just 3 lists (for vertex, edge and polygon selections). It should have events, to handle any selection changes. We could make one class for this, called "TPrimitiveSelectionHelper".
We could also split this up into 3 classes (each list has one).

Now, let's make a super-class called "TPrimitive" that contains these 3 objects as private fields. This class is just a "mediator" and allows the 3 subclasses to work with eachother.

This is just a quick draft for a possible design. I just like designing these things. Very Happy

The big challenge would be to break down the TBasePrimitive class, without breaking a lot of features.
Back to top
View user's profile Send private message
Jeroen
Site Admin


Joined: 07 Aug 2004
Posts: 5332
Location: The Netherlands

PostPosted: Sat Jan 23, 2010 8:54 am    Post subject: Reply with quote

The SearchOffset values: they are used when selecting things in 2D views. Imagine a cube in the top view. You only see a rectangle in that case. You're in polygon mode. Click once, and you select the upper polygon. Click again, and the lower polygon is selected. The selection handling searches in the polygon list of the primitive, starting at a specific index. This index needs to be preserved between calls to the selection code. I'm all in favor of placing them in separate datastructures. The same goes for any other kind of state variable.

The selection routines: sure, put them in separate classes as well. I'm very much in favor of the single responsibility pattern these days. Smile Be aware of the already existing Selector classes (EdgeSelector, PolygonSelector etc). Those work on collections of primitives and call the selection routines from the baseprimitive. They will have to be changed as well if you refactor this piece of the code.

If I was to redesign the base code, I would most likely end up with something similar. I have been thinking about refactoring this in the past but decided it was too much work given the limited time I have. Which brings me to a note of warning:

While developing DeleD, I have refactored large parts of the project over time as I learned more about OO programming and designing. Refactoring always costs a lot of time. Often more than anticipated because it is difficult to see which parts are affected, even for me as the original developer. Important: keep in mind that the user does not really benefit from it directly. They don't care if the code is all shiny, they just want new features or have bugs fixed.

Bottom line: I'm all for refactoring this code and I like the proposed design. I know there are other parts I would like to see improved greatly too (material forms come to mind) where code quality is concerned. Just ask yourself if it is really, really needed (because it is not maintainable or something) or if you just want to be an impatient perfectionist. Wink Or in other words: there are a lot of applications out there that never come to market because the developers are never satisfied with their code. Don't let this happen to DeleD, please.

I'm eager to learn what other developers think!
_________________
Check out Figuro, our online 3D app! More powerful 3D tools for free.
Back to top
View user's profile Send private message Send e-mail Visit poster's website
chronozphere
DeleD PRO user


Joined: 20 Jun 2006
Posts: 1010
Location: Netherlands

PostPosted: Sat Jan 23, 2010 9:17 am    Post subject: Reply with quote

I'll leave it as it is for now. Smile

However, keep in mind that the geometry structure is one of the core elements of DeleD and is used by alot of modules. If we want to write great and easy to understand tools, this code needs to be well designed.

Quote:

Important: keep in mind that the user does not really benefit from it directly. They don't care if the code is all shiny, they just want new features or have bugs fixed.


I agree, but not refactoring the sources now and then, is not an option either. The system evolves and new functionality is added. The core should evolve too, to allow new functionality to be included more easily. Eventually, we will hit a barrier because things have grown too complex.

When I start actually making these changes, I'll just try to split up the functionality. When that has been done, we will see what needs to be improved. I'll probably need some kind of a separate branch, because it'll take some time to fix this (Maybe I can use Git locally here, to resolve any conflicts before committing).

Just out of curiosity, how old is the TBasePrimitive code? Smile
Back to top
View user's profile Send private message
Jeroen
Site Admin


Joined: 07 Aug 2004
Posts: 5332
Location: The Netherlands

PostPosted: Sat Jan 23, 2010 10:22 am    Post subject: Reply with quote

chronozphere wrote:
I'll leave it as it is for now. Smile

However, keep in mind that the geometry structure is one of the core elements of DeleD and is used by alot of modules. If we want to write great and easy to understand tools, this code needs to be well designed.

I agree, but not refactoring the sources now and then, is not an option either. The system evolves and new functionality is added. The core should evolve too, to allow new functionality to be included more easily. Eventually, we will hit a barrier because things have grown too complex.

Just out of curiosity, how old is the TBasePrimitive code? Smile


I think we very much agree with eachother. Smile It's all just a matter of deciding if something really needs to be refactored. For example, I would not refactor the CSG code anytime soon because I don't foresee any functionality added anytime soon. So, the current code serves the user just fine, and the coders don't have to worry about it too much (except for solving the occasional bug). I would, however, think about redesigning the TBasePrimitive class, for reasons you already mentioned. Again, I'm anxious to see what other developers think!

The TBasePrimitive code has been there from the start, it just has grown over the years. I added new stuff without worrying about things like single responsibility too much, mostly because I was unfamiliar with many design principles at the time and I just wanted things to work. Over time, as I learned more about good design, I have refactored parts of DeleD into what it is now, knowing that refactoring is a never-ending story as time goes by, new functionality is added and new needs (from both users and coders) emerge.

Anyway, so what do other developers think? Should we refactor the TBasePrimitive code?
_________________
Check out Figuro, our online 3D app! More powerful 3D tools for free.
Back to top
View user's profile Send private message Send e-mail Visit poster's website
chronozphere
DeleD PRO user


Joined: 20 Jun 2006
Posts: 1010
Location: Netherlands

PostPosted: Sat Jan 23, 2010 11:18 am    Post subject: Reply with quote

Quote:

Again, I'm anxious to see what other developers think!


So am I. Smile

It just occured to me, that if we are to refactor this code, we should design it to work with the scripting feature that Paul proposed. Smile
Back to top
View user's profile Send private message
Paul-Jan
Site Admin


Joined: 08 Aug 2004
Posts: 3066
Location: Lage Zwaluwe

PostPosted: Sat Jan 23, 2010 2:10 pm    Post subject: Reply with quote

- Sure, the code can use tidying up. Itīs made in spare moments and after hours and with extremely tiny amounts of time invested (compared to daytime office software development).
- The question is, is refactoring more important than improving the functionality? User don't care about refactoring. They do care if you break existing functionality. Which will happen, unless there is a massive testing framework (either human or machine) available. But if we had that, the code wouldn't have needed refactoring in the first place. Smile
- I think refactoring is only relevant when the current structure directly hampers progress too much. In that case, refactor that piece of functionality.
- In general, if you can't refactor (and commit) in small steps at the time, you aren't doing it right.

Not trying to sound overly pessimistic here, refactor all you want, I'm just trying to prevent a situation where we have 3 people focusing on refactoring the code into a shiny ball of coding goodness ... that nobody uses, because the application itself has been standing still for years. Wink
Back to top
View user's profile Send private message Visit poster's website
Jeroen
Site Admin


Joined: 07 Aug 2004
Posts: 5332
Location: The Netherlands

PostPosted: Sat Jan 23, 2010 4:11 pm    Post subject: Reply with quote

Paul-Jan wrote:
... shiny ball of coding goodness ...


Laughing You gotta love that terminology. Cool

On topic: totally agreed!
_________________
Check out Figuro, our online 3D app! More powerful 3D tools for free.
Back to top
View user's profile Send private message Send e-mail Visit poster's website
chronozphere
DeleD PRO user


Joined: 20 Jun 2006
Posts: 1010
Location: Netherlands

PostPosted: Sat Jan 23, 2010 5:57 pm    Post subject: Reply with quote

Quote:

- I think refactoring is only relevant when the current structure directly hampers progress too much. In that case, refactor that piece of functionality.


You're making a good point. Personally, I really care about the code I'm working with. I'm a guy who re-starts projects because things have grown too complex. The downside of this, is that I never did any large projects myself (allthough they are getting bigger and bigger). The good part is that I learned how to write nice object-oriented code and that I always try to improve things.

When I see this code, something is telling me to refactor it (the same "something" told me to restructure the unit files. Laughing ).
So I am willing give this some thought and I'll probably do some local expiriments when I have time. We could all benefit from this. Smile

BTW: Please take a look at the three patches I uploaded!!! Very Happy Very Happy
A next DeleD release might not be far away.
Back to top
View user's profile Send private message
chronozphere
DeleD PRO user


Joined: 20 Jun 2006
Posts: 1010
Location: Netherlands

PostPosted: Thu Jan 28, 2010 10:02 pm    Post subject: Reply with quote

I just took a closer look at TBasePrimitive and some other core-classes.

My conclusion is: The functionality is good, but it's often not at the place where it should be, or it is not used at all.

This makes refactoring alot easier. Smile We just keep the same routines, but give them other (preferrably shorter) names and put them into appropriate classes. An example:

We now have this function:
Code:

    //Returns true if vertex is used by any of the edges in "edgeList"
    function TBasePrimitive.Edge_IsVertexUsedByEdges(edgeList: TEdgeList; vertex: TVertice): boolean;


I'd suggest we change it to this:

Code:

    function TEdgeList.IsUsing( Vertex: TVertice ): Boolean;


As you see it's much shorter. The function-name effectively describes the relationship between the EdgeList object and the Vertex parameter.
What do you think about it? Smile

I discovered quite some routines that don't seem to be used. I'd like to delete those (we can always put them back).
Back to top
View user's profile Send private message
Paul-Jan
Site Admin


Joined: 08 Aug 2004
Posts: 3066
Location: Lage Zwaluwe

PostPosted: Fri Jan 29, 2010 7:19 am    Post subject: Reply with quote

Go for it. As long as you pack your stuff into managable patches that we can review and commit more or less individually (i.e. don't create a single monster patch, but also don't create an endless chain of patches that all depend on each other) I'll happily merge things into the repository.
Back to top
View user's profile Send private message Visit poster's website
Jeroen
Site Admin


Joined: 07 Aug 2004
Posts: 5332
Location: The Netherlands

PostPosted: Fri Jan 29, 2010 8:31 am    Post subject: Reply with quote

Yeah, in lots of cases, refactoring is just a matter of putting things in more appropriate places, that's all. In other cases, breaking up code into separate pieces might be wise. Just remember earlier comments in this thread: only refactor when it's really needed. DeleD's codebase is good enough to add upon and I hate to see you working on DeleD for two months without actually adding new stuff users might like.
_________________
Check out Figuro, our online 3D app! More powerful 3D tools for free.
Back to top
View user's profile Send private message Send e-mail Visit poster's website
Display posts from previous:   
This forum is locked: you cannot post, reply to, or edit topics.   This topic is locked: you cannot edit posts or make replies.    DeleD Community Edition Forum Index -> DeleD Development All times are GMT
Page 1 of 1

 
Jump to:  
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum