DeleD Community Edition

chronozphere · Posted: Fri Jan 22, 2010 9:14 pm Post subject: The geometry structure

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?

Jeroen · Posted: Fri Jan 22, 2010 9:40 pm Post subject:

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.

chronozphere · Posted: Fri Jan 22, 2010 11:24 pm Post subject:

Jeroen · Posted: Sat Jan 23, 2010 8:54 am Post subject:

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.

chronozphere · Posted: Sat Jan 23, 2010 9:17 am Post subject:

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.

Jeroen · Posted: Sat Jan 23, 2010 10:22 am Post subject:

chronozphere · Posted: Sat Jan 23, 2010 11:18 am Post subject:

Paul-Jan · Posted: Sat Jan 23, 2010 2:10 pm Post subject:

- 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

Jeroen · Posted: Sat Jan 23, 2010 4:11 pm Post subject:

chronozphere · Posted: Sat Jan 23, 2010 5:57 pm Post subject:

chronozphere · Posted: Thu Jan 28, 2010 10:02 pm Post subject:

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:

Paul-Jan · Posted: Fri Jan 29, 2010 7:19 am Post subject:

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.

Jeroen · Posted: Fri Jan 29, 2010 8:31 am Post subject:

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.