Thoughts on refactoring and futher improving the engine

Started by Crimson Wizard, Wed 20/06/2012 09:46:17

Previous topic - Next topic

SpeechCenter

I would consider separating as much as possible Allegro from the data structures. For example, all the GUIs are dependent on it because they don't only hold this info, but also draw it.
This makes the Native dll is also dependent on Allegro. Thus, separating the underlying data and creating a different presentation layer would bring value.

This doesn't mean ability to replace Allegro (I think that's much more difficult and not sure if it's needed at this point), but it will be a step towards a better design.

tzachs

Quote from: Crimson Wizard on Wed 15/08/2012 08:29:55
Question is about the aim.
I think "baby steps" is the answer here. You should perform the minimal amount of steps that will get your code to work as the current engine works, and perform shitload amount of testing before thinking of the next development goal.

I've recently been encountered with the Mikado Method, and I think it's the best method to take with large scaled re-factoring.

Crimson Wizard

#82
Quote from: tzachs on Thu 16/08/2012 10:11:17
I think "baby steps" is the answer here. You should perform the minimal amount of steps that will get your code to work as the current engine works,
You mean "minimal amount of steps" or "steps of minimal length"? :)



//----------------------
MUCH LATER EDIT:
Reading that pdf you linked actually made me wonder if what I did for last month was actually good thing ;)
Now I have a feeling that probably I should return back in time to where I left "refactory" branch and continue working with it instead of rush-making totally new classes. I won't delete my work of course, and will be able to use it as a reference later.

Perhaps I should write the immediate goals somewhere too.

SSH

Sorry if this has already been covered, but I wondered if someone should write an artificial "game" that tries to exercise as much of the engine as possible which could then be used as some kind of automated regression test for the refactoring?
12

Crimson Wizard

Quote from: SSH on Thu 16/08/2012 17:14:10
Sorry if this has already been covered, but I wondered if someone should write an artificial "game" that tries to exercise as much of the engine as possible which could then be used as some kind of automated regression test for the refactoring?
You are reading my thoughts. I hoped someone suggests to do this, otherwise I'd had to post in the "Recruitment" section :).
I don't have enough time for this, and I will very much appreciate if someone makes such game.

Crimson Wizard

Are there any benefits in using structs when passing coordinates to drawing functions?

Random examples:
Code: cpp

CBitmap::DrawRect(CRect &rc, int color);
CBitmap::Blit(CBitmap *bmp, CRect &rc_src, CRect &rc_dst);


As I see this, advantages commonly are: the parameters are grouped and hence it is easier to understand what's what, less likely someone will make a mistake.
On other hand, this makes writing little longer when you can't use struct right away and have to init CRect right at function call, some may say this makes code more cluttered.
What do you think?

Calin Leafshade

Personally I prefer passing data structures when parameters are related.

A point x,y is defined as a point and thus you should pass the point if its the point that the function is interested in because that makes the most semantic sense to the function.

This might make code a little longer but it always makes perfect semantic sense with regards to what is actually happening which i would argue is better.

JJS

As an interface developer I like the elegance of the CRect approach. But as a user of the interface I hate it because 90 % of the time it makes life harder and causes unnecessary code bloat without offering a benefit.

I guess it would be better if the most usual cases (whole input image and whole output image) could be accesses without having to construct the rect yourself. This could be done by overloading the method. Well, or if CBitmap had a member variable holding its size.

All that said, I would go with CRect.


Something else: I am still in the process of making the refactoring branch compile for other platforms. At the moment I am taking care of Android. There is a very annoying bug in the Android toolchain that makes it impossible to include "debug/debug.h". Instead of using the files in the user include path it will include the respective file from the standard library (wrong) So I have to rename the file. Any suggestion for a new name? Maybe "debug/agsdebug.h"? I know this is stupid, but I cannot help it.

Also I am strongly considering switching development to the refactoring branch. The original code is a mess and it gets worse every time I hack something in. An additional benefit would be that the refactored code is actually used and therefore checked for errors.

If I switch these are things I would like to do:

Remove unnecessary code:
- Memory allocation check because it is a joke. 4 MiB is not enough to load any game no matter how small it is.
- Path finder copyright "protection"

Refactor these:
- Moving the plugin interface into there own files and partially into the platform files. Right now it is totally backwards with the platform files calling the generic implementation which is then a nasty ifdef mess for all platforms. Also Linux plugin implementation is broken and the plugin stubs break games if they use the same script function calls.
- Removing the platform config file reading code and putting everything into acsetup.cfg albeit making the engine configurable on all platforms. Right now Windows, Linux and OSX don't have the advanced options of the other ports.
- Adding the OpenGL driver to the graphic driver list. Also adding the D3D render-to-texture driver.
- Removing the variables with which the port frontends control things like the game file. They should be replaced by constructing parameters to main().

Edit:
- Replace Sleep() calls with platform::Delay() everywhere. I already did that in the code I am working on.
- Replace the debug output system ASAP because it is a hassle to do debug output in a lot of files since they don't have access to the platform header and VS only understands OutputDebugString while *x*-like stuff needs printf and Android again its own thing and then my head explodes.
Ask me about AGS on PSP, Android and iOS! Source, Daily builds

Crimson Wizard

It is CRect then! (with overrides for whole bmp blt).

Quote from: JJS on Thu 23/08/2012 14:19:43
I am still in the process of making the refactoring branch compile for other platforms. At the moment I am taking care of Android. There is a very annoying bug in the Android toolchain that makes it impossible to include "debug/debug.h". Instead of using the files in the user include path it will include the respective file from the standard library (wrong) So I have to rename the file. Any suggestion for a new name? Maybe "debug/agsdebug.h"? I know this is stupid, but I cannot help it.
The 'debug/debug' unit contains logging functions and helper functions that communicate with Debugger. In such case I propose to move logging functions to 'debug_log' and debugger helpers to 'debugger'.

Quote from: JJS on Thu 23/08/2012 14:19:43
Also I am strongly considering switching development to the refactoring branch. The original code is a mess and it gets worse every time I hack something in. An additional benefit would be that the refactored code is actually used and therefore checked for errors.
Makes good sense. I am already finding some minor mistakes I did. :/

Quote from: JJS on Thu 23/08/2012 14:19:43
- Replace the debug output system ASAP because it is a hassle to do debug output in a lot of files since they don't have access to the platform header and VS only understands OutputDebugString while *x*-like stuff needs printf and Android again its own thing and then my head explodes.
I posted this a while ago: http://www.adventuregamestudio.co.uk/forums/index.php?topic=46290.0
Any thoughts on that?


Regarding myself, I switched back to work in my personal repository because what I do now is making several parallel branches at once (replacing strings, file streams and bitmaps with new classes) and I do not want to clutter adventuregamesstudio repo. Later I'll merge them in one and pull to central refactory branch.
I'm taking tzachs's advice about little step, although that may mean some of the code I do now will be later replaced once again.

Snarky

I agree with JJS's first instinct. While the semantics and "elegance" makes it tempting for an API designer to wrap a pair of coordinates as a Point data structure, or two pairs as a Rectangle, for the user of the API it's simply a bunch of unnecessary extra work, where you're usually wrapping your values in a struct for no other reason than to pass the argument, which the function then immediately unwraps on its side. It clutters up the code with data wrangling and obscures the more important logic of what you're doing.

The relationship between a pair, or two pairs, of numbers is simple enough and standard enough that you don't need a struct to explain it to you. Particularly when the IDE provides a pop-up explaining that the arguments are (x1, y1, x2, y2). Using data structures for parameter-passing is only helpful if the setup is complex or spread out over time, or you're likely to use the same set of parameters in several separate method calls, or it's non-obvious how the different arguments relate to each other, or when there are so many of them that it would be confusing to set them all in one single line.

The CRect version should only be an optional overload, in my opinion.

BigMc

Quote from: JJS on Thu 23/08/2012 14:19:43
Also I am strongly considering switching development to the refactoring branch. The original code is a mess and it gets worse every time I hack something in. An additional benefit would be that the refactored code is actually used and therefore checked for errors.

Yay! Smells like progress. :)

SSH

Of course, if the entire parameter set is a list (like perl) then you could perhaps have the possibility to do both:

Code: AGS

stair_start=(127,204);
stair_end=(175,156);
stair_area=(stair_start, stair_end);
call_some_function(stair_area);
call_some_function(stair_start, stair_end);
call_some_function(127,204,175,156);

12

JJS

Anyone opposing a beautifying of the whole code? I mean running a tool over it that cleans up the formatting.

I know this is controversial because you essentially lose the history. But: If we don't do it now it will only get worse. Right now the history is basically useless already due to all the splitting. There is no "perfect" time to do this, but I think now is the least bad.

Also this makes further meaningful changes more apparent in the history because they are not always intertwined with formatting changes.

I would only beautify the engine/common cpp, c and h files and not the library sources.

Thoughts?


Edit: I am still trying to build for Android, but the toolchain really likes to mess with me... The linker gives an undefined reference error for a symbol that is clearly defined in a source file that gets build. :-X

Edit2: Are you kidding me? The file is shown as being successfully built while in reality the compiler creates an empty object file.

Edit3: Fixed I guess. It's always the most stupid thing.
Ask me about AGS on PSP, Android and iOS! Source, Daily builds

Crimson Wizard

Quote from: JJS on Fri 24/08/2012 14:54:10
Anyone opposing a beautifying of the whole code? I mean running a tool over it that cleans up the formatting.
Heh, I was arguing about this with Alan v. Drake some time ago :).
My main argument against doing that was that I have to copy changes from 'main' branch.
But since a) we are about to move to refactored code anyway, b) it does not look like we are copying any big changes (and any changes at all) from "official" SVN or elsewhere, c) it is actually unknown how long it will take to rewrite everything --- I think my argument does not have much weight anymore.

Quote from: JJS
Edit: I am still trying to build for Android, but the toolchain really likes to mess with me... The linker gives an undefined reference error for a symbol that is clearly defined in a source file that gets build. :-X
Edit2: Are you kidding me? The file is shown as being successfully built while in reality the compiler creates an empty object file.
That sounds similar to Visual Studio bug that could not compile the second file of same name but compiled the first one instead :) Don't know if the reasons same though.

BTW, again about debug output system I mentioned above. Have anyone looked at it? I want to merge last changes from main to refactory on this weekend. If there are no objections I will also copy output system there.

JJS

I am checking out the debug system right now. What I can tell is that there is an exception in the destructor of OUTPUT_TARGET. It is caused by previously deleting the delegate_objects but not setting their pointer to NULL. I would propose changing the SAFE_DELETE macro to

Code: c++

#define SAFE_DELETE(p)          if (p) { delete p; p = NULL; }
#define SAFE_DELETE_ARR(p)      if (p) { delete [] p; p = NULL; }


which fixes the problem and gets rid of dangling pointers.

(I guess you have that already fixed in a private version, but o well...)


Edit: I wanted to try out writing a new debug output class that prints to the screen quake console style. But I realised that this is not possible without either hacking into the engine or inventing some sort of event callback system. But wait! There is already one present for the Plugins. If it was generalized a bit it could be used for in-engine purposes like that...
Ask me about AGS on PSP, Android and iOS! Source, Daily builds

Calin Leafshade

There is already a debug console, quake style.

Press ` (key left of 1) in game with debug mode on.

Crimson Wizard

#96
Yeah, there's some debug console in AGS already, as it looks like from the code, but funny thing is I don't remember ever seeing it in work.
CConsoleOutputTarget class is supposed to write there, but to my shame I now recall I haven't tested that.

As for errors in output system - well, I'll certainly recheck everything there.

EDIT: @JJS, yeah you are right about SAFE_DELETE. Dunno how I missed that one. Thing is that delegate is deleted both from destructor and also out::shutdown()

JJS

The more you know... although I have an excuse for not knowing: On German keyboards the normal quake key (next to 1) is "^". Because this is a composite key so that you can do stuff like â and î it is not recognized by AGS at all. And to enter "`" you have to press SHIFT+"´". So I suppose this feature is broken on German keyboard layouts.

Still, for true encapsulation of the whole debug output method an event system is necessary.
Ask me about AGS on PSP, Android and iOS! Source, Daily builds

Crimson Wizard

Quote from: JJS on Fri 24/08/2012 17:30:07
Still, for true encapsulation of the whole debug output method an event system is necessary.
Do you plan making each game entity send event messages, or something simplier (or totally different)?

Meanwhile, updated refactory from main.
Tested in a small game, new roomstats code looks working so far.

Alan v.Drake

Ahah, yeah, the mystical console, you have to set the keyboard layout to EN so when you hit the key under esc it works. I realized it existed only early this year.

I have no qualms against beautifying the code.

You guys have really made an atounding work on the source, I feel shamed I haven't finished porting my changes yet :(

- Alan


SMF spam blocked by CleanTalk