Thoughts on refactoring and futher improving the engine

[Crimson Wizard]

Here are my thoughts on possible continuation of the hypothetical branch of AGS that would merge JJS's portable version and refactored one.
I wanted to wait till I clean my branch a bit before posting this, but that takes time and also monkey_05_06 told me he wants to work on that too; so it goes here.

Before anything else I want to state following:
1. Unfortunately I am not aware of development process in official AGS 3.2 branch. I only know that Tobihan merged it into JJS's rep some time ago, but that was only Editor code.
2. My own branch is based on JJS's one and has practically everything JJS and Tobihan made, except for maybe few recent commits. I planned to merge their code at every stable stage, but after last changes I made to my branch I am not sure I will be able to do so with the same ease, since code structure changed too much (on other hand, it is perhaps worth trying automatic merge to see what happens ).
Recently things became somewhat messy there because of rushed splitting large source file, but I am working on cleaning things up right now.
It does compile and seem to work as expected (i.e. without any changes to program execution). On Windows - that's it. Haven't tested on other platforms that are supported by JJS branch.
3. I asked JJS and Tobihan to tell their opinion about merging our branches; so far I got response from Tobihan who agreed to check and fix linux compilation for my branch.
I haven't got answer from JJS yet, and I would really like to hear from him.

Links:
Adventure Game Studio "organization" on github:
https://github.com/adventuregamestudio
contains JJS's branch:
https://github.com/adventuregamestudio/ags

My own branch:
https://github.com/ivan-mogilko/ags-refactoring


Following is not a "big plan", but rather thoughts on most immediate things needed.

Organizational
Obviously there should be a feature and bug tracker.
Using forum threads is not a good idea, since they tend to become messy and off-topic too fast.

JJS has one for his own branch: http://jjs.at/tracker/
Does the offical AGS branch have it?
What may we use?

Some place to discuss practical coding problems. I know this will sound snobish, but I am a pragmatic man and I realize that doing this on forums with public write-access will make it mostly off-topic chat eventually as well.

Code design documentation.

Administrative
Since there will be alot of ideas thrown in eventually, I believe we must determine the criteria which would allow us to split all such ideas and feature proposals into three categories:
- Ones that change engine/editor too much to be implemented into version 3.* and should be left aside until better times (AGS 4).
- Ones that could be implemented into 3.* without breaking backwards compatibility.
- Ones that could be implemented into 3.*, but are too exotic or not as useful to average game creator to be part of engine core, and thus should be rather made as plugins.

Code-wise
Code-style is to be determined. I do not think it should be thoughtlessly applied to existing code, since that will increase amount of work needed to merge changes from other branches, but as we rewrite parts of it we should use code-style.

Following are changes that I would really like to see made to AGS engine:
1. Separating platform-dependent and low-level code as much as possible (and reasonable) from engine core.
There should be no #ifdef WINDOWS_VERSION or #ifdef ALLEGRO_BIG_ENDIAN scattered all around the code as it is now.
I belive, there should be no raw FILE or BITMAP type references in the engine core as well. I hate to include allegro headers in almost every unit only because one of the function it needs to be known has BITMAP or RGB in parameter list.
Probably writing interfaces for some kind of "serializer" or abstract drawing system will improve things.
2. Substituting raw static and dynamic arrays with classes. That could be either STL or AGS own utility classes.
String class too, perhaps.



//--------------------
EDIT: Oh, right, how could I forgot...
Important thing is to decide whether to propose refactored branch as a merge candidate to official 3.2 or not.
Ofcourse it needs more testing, but with changes made only to code composition and not to program execution it does not make whole new version of AGS, like 4.0.
I would suggest to aim for version 3.3 or something like that, which would include refactored code + some of the most wanted additions to engine.

[JJS]

Before anything else I want to state following:

1. Everything that was in the 3.2.2 dev branch of the SVN was merged. It was all editor changes and I am not aware of official engine changes.
2. I don't think merging is hard since large code changes are mostly in the platform specific code which is untouched by the refactoring. The engine changes are usually very small.
3. If by merging you mean that the main branch switches to the refactored code: Not sure if this is beneficial at this stage. But I will also test the changes on the other platforms.
What we could do is creating a refactoring branch directly in https://github.com/adventuregamestudio if that helps.


Organizational:
Github has a bug tracker. It would be the best choice imho since it is closest to the source code.


Code-wise:
1. YES. The various platform defines really make the code error prone. For example I was hunting some drawing error that stemmed from an innocent #ifdef MAC_VERSION hidden somewhere. I think #ifdefs should be rewritten so that they focus on a specific task/property instead of a platform. E.g. having a define for RGB vs. BGR color order.
Existing platform specific methods should also be used, e.g. for delaying the execution there is a method in the platform class but still Sleep() is used in various places and has to be separately redefined for non-Windows platforms.

[Crimson Wizard]

3. If by merging you mean that the main branch switches to the refactored code: Not sure if this is beneficial at this stage. But I will also test the changes on the other platforms.
What we could do is creating a refactoring branch directly in https://github.com/adventuregamestudio if that helps.

My primary intent is to make this more open and easier to find so others could join in and collaborate eventually.
So, I would totally agree if there will be a separate branch in adventuregamestudio for now.

Existing platform specific methods should also be used, e.g. for delaying the execution there is a method in the platform class but still Sleep() is used in various places and has to be separately redefined for non-Windows platforms.

Well, my idea in general is to limit the amount of modules that "know" they are using something platform-specific. If a code uses a symbol without having to "worry" whether it is a Windows function or some defined alias with extra tweaking, it should be ok, I guess.

[AGA]

There is a project tracker built into the forums, it's just that until now no one except moderators could see it.  I was waiting for the moderators to take a look at it and sign off, but no one ever did!

Assuming this seems vaguely fit for purpose I'd be happy to set some people up as project administrators so they can fiddle with the options.  I don't really pay any attention to AGS dev though, so who do people think should be in charge?  There is the option to have multiple projects at once, so you could have one for each engine fork, each IDE fork and so on, with different admins (and access / visibility levels) for each.

[monkey0506]

Based on the thread that I started some time ago, Dave Gilbert, m0ds, and myself stood out as candidates for overseers of AGS as a whole. Myself, Wyz, and JJS were all nominated for technical lead. Others who have check in rights like Calin Leafshade and ProgZMax would also probably be good to have added there. Interesting that this is built into the forums. That actually improves things a lot I think. We should work on copying over the old bug/feature tracker and maintaining it.

[AGA]

Well I do have access to all the site databases as well as the forums'.  I'll take a look and see if I could restructure the old bug tracker data and insert it into this new system.

I'll give this, and the access rights, a look this evening.

[monkey0506]

You're rather sexy for a bazooka-wielding wolfman, you know that?

[AGA]

Can someone give me an idea of what projects we would like set up, and who should administrate them?  I could just give all of the people you mentioned admin for the time being, and let you sort it out yourselves, but that might lead to confusion with so many people trying to do the same work at the same time.

[JJS]

About the repository: I gave Crimson Wizard commit rights to it. monkey_05_06, let me know when you sign up and I will add you too. Also anyone else interested is very welcome.

So how do you want to organize this? Do you want to pull all of the branches of the refactoring fork into it or just the latest one? I am honestly not well-versed into how to do that correctly.

As for the forum builtin bug tracker: That is pretty cool! And I would be for just giving everyone admin rights. I think the risk of nobody stepping up for it now is higher than that of people invalidating each others work.

[Crimson Wizard]

JJS, thank you! (Now I need to be careful not to screw something up there )

Regarding branches.
So far my work was mainly straightforward and used branching for the purpose of dividing history into stages rather than separating concurrent tasks.
This means that although there are 6 general branches right now, the last one (refact_Stage_4) is only needed, but it also "contains" all previous ones. (There are couple of side-branches too but they were used for temporary holding some code I later merged into main branch anyway).

More important question is, where do I pull it to? Should I create a separate repository inside organization, or a branch from ags/main?

//-----------
@monkey_05_06,
Earlier you spoke about implementing base utility classes. Perhaps it could be the next logical step. I need to explain one organizational issue though.
I do not know yet how the future work will be planned, but until now I tried to divide my work into stages, each for one general task; I also made sure everything builds and runs in most of times (and especially at the end of each stage).
1. Stage One: splitting the code into separate projects (two libraries - Common and Compiler - and one Engine application) with strict dependancy; then making shared library compile on its own without including any engine or editor-specific code.
2. Stage Two: splitting the code in Common and Compiler libraries into smaller units.
3. Stage Three: splitting engine code into smaller units.

I am currently at Stage 4, which is:
- tidying up;
- making sure all source files are in corresponding directories (Engine dir contains only engine files, Common dir only shared files etc);
- making sure that all the ports that work in JJS fork work on mine as well.
At the end of Stage 4 I am planning to merge all the differences from JJS branch.
This means that I'll have a code which was restructured, but executes at least 99% same way as the one in JJS branch.
I prefer to then leave that branch as a stable reserved source in case we screw up something in the future.

I think it is better to make new branch for any larger changes, like implementing new classes and similar stuff. What could it be named - Stage_5 or somehow more originally - I don't know. I am not sure that could be called refactoring if we put new utility classes (like STL) to use, since that will actually change the way program runs. Or, maybe it's just a not so important matter of preference and naming conventions.
What I intend to say, I guess you can branch from refact_Stage_4 right away and start writing classes or substituting C-style arrays and strings with std:: classes. When I finish Stage 4, I will just merge the changes to yours, or vice versa.

[AGA]

I'm working on importing the old bug and feature suggestion list.  As soon as I've got that done, I'll give some people admin so you can start using this properly.

[JJS]

^Thanks!

I guess the branch has to be in the "ags" repository. Otherwise github will not understand that it is related to the the main repository which would be bad for merging (probably)?

So a new branch in "ags" it is.

[Crimson Wizard]

It appears one cannot make a pull request to non-existing branch (i.e. to create a new one).
This leaves me one option - to just push my local rep to adventuregamestudio/ags/-new-branch-name-.

UPDATE: hey, that worked, it even shows the history correct (my branch forked from main in the past):
https://github.com/adventuregamestudio/ags/network

[BigMc]

I guess that's what you wanted to do:

Code: AGS

git remote add ivan git://github.com/ivan-mogilko/ags-refactoring.git
git fetch ivan
git checkout -b refactoring ivan/refact_Stage_4
git push refactoring

It's all quite easy, just use google.

[Crimson Wizard]

Yeah, I sort of did similar thing

[Joseph DiPerla]

About the repository: I gave Crimson Wizard commit rights to it. monkey_05_06, let me know when you sign up and I will add you too. Also anyone else interested is very welcome.

So how do you want to organize this? Do you want to pull all of the branches of the refactoring fork into it or just the latest one? I am honestly not well-versed into how to do that correctly.

As for the forum builtin bug tracker: That is pretty cool! And I would be for just giving everyone admin rights. I think the risk of nobody stepping up for it now is higher than that of people invalidating each others work.

I do not want to put anyone on the spot, but perhaps giving priviledges to Calin Leafshade  and Alan V. Drake would be a good idea. Calin seems to be talented when it comes to programming (I believe he worked on an AGS to X-Box converter, no?) and Alan has a pretty neat version of AGS Draconian Edition r6. Would be cool to have all you guys working on that. And do not forget CJ of course

So JJS, is Github your new home for development or are you still using gitorious?

[JJS]

The repository is now on Github, so I will not update on gitorious anymore.

Anybody who wants to work on the source is welcome. There is also always the possibility of forking the source, working on something and then creating a pull request.

[monkey0506]

monkey0506 is now registered on github and watching the AdventureGameStudio/AGS repo.

[JJS]

Nice, I've added you.

[monkey0506]

Thanks JJS!

For quite some time I've been putting a lot of time and effort into reading about various different pros and cons regarding different string class implementations. It didn't get a lot of replies (but I only searched for 0.03 seconds on Google ), but this thread on the ScummVM forums raises some interesting and potentially compelling points about implementing a string class for the AGS engine.

I think that a proper string class would not only make the code much more readable, but also much safer than just using char*s. Based on some things I've read, there's even ways that we could optimize usage to actually make a string class better than just using raw char*s. As a case in point, I've recently been reading up on different ways to find the length of a string, and come up with the following:

Code: cpp

#include <iostream>
#include <string.h> // included for strlen
using namespace std;

template <size_t N>
inline void length(const char (ext)[N])
{
    cout << endl << "array version: " << text << ", Length: " << N - 1 << endl; // N includes '\0'
}

inline void length(const char*& text)
{
    cout << endl << "pointer reference version: " << text << ", Length: " << strlen(text) << endl;
}

int main()
{
    length("Hello World!");
    const char s[] = "Hello World!";
    length(s);
    const char *t = "Hello World!";
    length(t);
    return 0;
}

The calls with the string-literal and the char[] both call the "array version" (at least they do on GNU GCC), and the call with the char* calls the "pointer reference version". From what I've read, most compilers would likely optimize these functions out of existence entirely (especially with the inline specification). Given that, a template version of a string length function could be optimized to a constant value if given a string literal or char[]. It would only have to actually call the strlen method if the actual parameter was passed in as a char*.

Forgive me if I'm being daft, but wouldn't that be better than what strlen is doing?

Code: cpp

size_t strlen(const char *s)
{
  size_t len = 0;
  while (s[len] != '\0') len++;
  return len;
}

This is the type of thing I'd be interested in implementing into the engine code. Please let me know your thoughts.

Edit: I did forget that a char[] has to have a size defined when it is (even if it's determined at compile-time rather than a static value). Hmmm...I'm still looking to see how I could take further advantage of this (although the benefit with regard to string literals isn't negligible IMO).

[JJS]

I think the length() function would be broken for cases like this:

Code: c

char buffer[100];
strcpy(buffer, "hello");

The actual length is 5 but the function would return 99, wouldn't it?

[Crimson Wizard]

Well, as being said

a char[] has to have a size defined

In other words, that would work only when compiler can determine array size on its own.
This would be useless for char pointers when it is unknown what do they point at.

Also, I am not sure, how does this relate to a string class? String class is usually supposed to store string length (at least I think it should).

I have this proposition: make a simple and robust string, vector, etc classes. I my opinion defining their interface is more important at the moment, for implementation could be optimized and extended in future if wanted, but interface changes will require rewriting lots of code which uses them.

I should underline, that, in my opinion, a problem of AGS code is not only that it is uses unsafe arrays and strings, but also that it usually makes most of operations with them in place (e.g. string copying), thus increasing code size and repeating same simple algorythms over and over again all around.

[Snarky]

Regarding strings, please please please keep in mind to make the API Unicode compatible (UTF-8, presumably), so that one day we can support more than 128 (or 256) characters.

In any case, aren't there string libraries out there already? Not much point reinventing that particular wheel, is there?

[Crimson Wizard]

Regarding strings, please please please keep in mind to make the API Unicode compatible (UTF-8, presumably), so that one day we can support more than 128 (or 256) characters.

It's good you mentioned that, it seems I forgot about this completely  At the moment I don't know how much problematic would be to start using wide strings in the engine, especially in the light of script system and backwards-compatibility. We need to check that out.

In any case, aren't there string libraries out there already? Not much point reinventing that particular wheel, is there?

There *may* be portability issues, plus std::string (as well as STL in general) generates a lot of code, as was mentioned in the thread monkey gave a link to.
But I am sure there should be a good free string library somewhere in the web... I am wondering if monkey is looking for one or wants to write that himself no matter what...

To be completely honest, string and array classes are not so difficult to write, if you aim for practical simplicity and not some uber-universal classes.

[monkey0506]

I think the length() function would be broken for cases like this:

Code: c

char buffer[100];
strcpy(buffer, "hello");

The actual length is 5 but the function would return 99, wouldn't it?

That's a fair point, but that's why we would need a proper string class to begin with. Cases like this should be avoided altogether. If it's completely unavoidable, we could force the array to decay into a pointer. Based on some preliminary testing, there is actually (although admittedly small on my particular system) some benefit behind the static length in the case of string literals. This obviously doesn't benefit if it's stored in a pointer first, or once it's actually stored into the string class, but if you construct a string object using a string literal then having a static size is more efficient than having to parse it, looking for the '\0'.

I'm not against using a pre-existing string class (there are a million of them), but it wouldn't hurt to take into consideration exactly what AGS itself needs, and consider just writing our own. As CW said, it's not that they're difficult to write. Besides that, writing our own would also help future-proof code against licensing issues, portability, and so forth.

UTF-8 in particular would be a good case to work with, because then we wouldn't need to worry about using "wide characters". That is, wchar_t is not very well defined from compiler to compiler or between different system architectures, so using the 8-bit UTF-8 would allow us to stick to regular char and still accommodate international characters. I'm sure we would need to make some special handling to get an entire UTF-8 character (which could be represented by anywhere from 1 up to 6 4* chars/bytes). *Still reading about the UTF-8 specification, originally it could handle characters up to 6 bytes, but that was changed in 2003, so the official specification will only take up to a maximum of 4 bytes. Of course with the way the specification marks the bytes, it would be easy to back-track to the head byte of any given char in the string.

Edit: I was just thinking about it, and if we use UTF-8 strings, then "Length" should probably represent the actual number of UTF-8 characters rather than the number of chars used to store them. Based on my readings, it is a misconception to think that allocating larger blocks is a worse alternative than having to reallocate every time the string is changed. If we did implement our own UTF-8 string class, I'm thinking that "Length" should probably represent the actual UTF-8 character count, something like "CharLength" or "CharCount" should represent the number of chars in use, and "Size" should represent the allocated size of the buffer. This would probably end up negating the proposed benefit I mentioned before with regard to string literals, as both the UTF-8 length and actual char count would both need to be calculated. Oh well, just trying to be the very best, like no one ever was. To make strings is my real test, UTF-8 is now my cause.

Also, I've been looking and I'm not aware of any well defined portable methods of casting a floating-point value (particularly doubles) into a string/char*/char[]/etc. I've come up with something that can pull up to 10 digits into an integer and return the given precision (to distinguish between say 0.5 and 0.05). It seems to be rather functional (better than other implementations I've seen in any case).

[AGA]

Okay, so the old bug tracker's info is now imported, as 'issues' in the new system.  The information isn't perfectly transferred, but the way CJ wrote the old bugtracker was silly.

I have set up:

Dave Gilbert, m0ds, monkey_05_06, Wyz, JJS, Calin Leafshade, ProgZMax

as admins for the Adventure Game Studio project.  If at some point we want to make up different projects (AGS iOS, AGS Linux etc), please let me know.

[Crimson Wizard]

If at some point we want to make up different projects (AGS iOS, AGS Linux etc), please let me know.

Should there be a differentiation between development branches?
If I understand this right, there are at least three:
SVN branch (3.2 something)
JJS branch with ports which is obviously far away from SVN
my branch with refactored engine, which is - so far - about same program as JJS's, but with additional risk factor.

Now, if people find bugs in one branch, they won't be necessarily applicable to another.
And yet there's a question about feature requests. Consider something was asked to be added to AGS (editor or engine), where should it be implemented? I think this should be discussed a bit further.

[tzachs]

My thoughts on this:
The official version is SVN Main, and so the tracker should track bugs regarding the main SVN branch, not the unofficial versions, and I'm not sure about the development branch, but I'm tending towards no to that as well.

Eventually we will want to merge those unofficial ports into the main trunk and release an official version (the sooner the better IMO), we wouldn't want to keep those branches alive, this could create chaos for the exact reasons you mentioned.

[Crimson Wizard]

Eventually we will want to merge those unofficial ports into the main trunk and release an official version (the sooner the better IMO), we wouldn't want to keep those branches alive, this could create chaos for the exact reasons you mentioned.

I must disagree on what you say about "chaos". I don't think the situation I mentioned will necessarily cause chaos; it won't if the criterias for feature implementation are defined (i.e. what is allowed to go where).

But what I would really want to know: every development has certain goal. Was there any goal defined for the next official version (aka SVN Main)? Was it reached? If not, who continues to work on that?
Same for dev 3.2.2. What were your goals? Were they reached?
I am asking because I can hardly understand what's going on there, or rather why nothing is going on there.

I would like to also hear others on this. But from my position, that is - based on what I see and know, - it would be more logical to follow this release pattern:
- test and polish dev 3.2.2, and release it as 3.2.2; if there are any features added or fixes to engine and/or editor they are to be also merged to JJS's branch afterwards;
- finish, test and polish JJS's branch and release it as, for example, 3.3 (we could call it "AGS portable" or something);
- merge everything from JJS's branch into refactored branch and continue to develop it; perhaps release it as some 3.3.1 or 3.4 as an "experimental" release first, before real serious changes are made.
But that's a mere suggestion. I am still lacking understanding of everyone's plans...

[BigMc]

If you want to stick to a repository on CJ's server and import stuff there before a release, ok. But please then install git on that server. If you have worked with both, you know that git is technically better and nobody should import stuff into svn anymore.

[AGA]

Running a local Git server is an option.

[timofonic]

Running a local Git server is an option.

What about running one in adventuregamestudio.co.uk subdomain? Like git.adventuregamestudio.co.uk

[AGA]

It's certainly an option.

[timofonic]

It's certainly an option.

Would CJ agree on converting the SVN repository to Git? After all, most active devs are using it so merging/forking would be easier and such.

(a dump of the original visual source safe repository would be appreciated to integrate it and save the code progress, for historical and retrocompatibility research reasons...)

[Joseph DiPerla]

Running a local Git server is an option.

What about running one in adventuregamestudio.co.uk subdomain? Like git.adventuregamestudio.co.uk

I was actually thinking this would be a good idea too.

[Crimson Wizard]

[breaking news]

SVN repository, Common and Engine folders
*.cpp files count: 37

"refactory" branch, Common, Compiler and Engine folders
*.cpp files count: 260

Dum-dee-dum 

[/breaking news][/silly mode off]

[Crimson Wizard]

I am close to finishing the "clean up" stage of refactoring (may take few more days).

Meanwhile I wrote an experimental code, introducing new classes (check commit changes to know what's new):
https://github.com/adventuregamestudio/ags/tree/pre_advance

EDIT: Just in case, I'll mention important units I added:
Common: core/err, core/out, core/types, platform/file, platform/path, util/keyvaluetree, util/stream, util/string;
Engine: core/engine, core/engine_setup, util/cmdargs, util/inifile;

I made the commit early so that others could check that out and tell what they think. If there are any wrong things it is important that they are known before we actually start putting this into use.
I am mainly interested in hearing opinions on coding style and general ideas.
Please, remember that most of new things there are either raw drafts or placeholders. The code may compile (I am pretty sure it will), but don't try to run it (it will do nothing or crash, I haven't actually tried, since that does not make any sense yet).

[Joseph DiPerla]

I am close to finishing the "clean up" stage of refactoring (may take few more days).

Meanwhile I wrote an experimental code, introducing new classes (check commit changes to know what's new):
https://github.com/adventuregamestudio/ags/tree/pre_advance
I made the commit early so that others could check that out and tell what they think. If there are any wrong things it is important that they are known before we actually start putting this into use.
I am mainly interested in hearing opinions on coding style and general ideas.
Please, remember that most of new things there are either raw drafts or placeholders. The code may compile (I am pretty sure it will), but don't try to run it (it will do nothing or crash, I haven't actually tried, since that does not make any sense yet).

:0

I am so glad there are guys like you and JJS helping out on AGS. Otherwise I would fear it would halt in development. GREAT JOB!

[Joseph DiPerla]

I keep getting this error when compiling:

Code: AGS

1>c:\Joeys fun projects\agssource-preadvance\adventuregamestudio-ags-33da32f\Common\platform/file.h(33) : fatal error C1083: Cannot open include file: 'Common/util/string.h': No such file or directory

I made sure I updated the VC++ directories to the new common folder, however, it still says its missing. I look in the folder and its there! So I have no idea whats going on. Idea's?

[Crimson Wizard]

I keep getting this error when compiling:

Code: AGS

1>c:\Joeys fun projects\agssource-preadvance\adventuregamestudio-ags-33da32f\Common\platform/file.h(33) : fatal error C1083: Cannot open include file: 'Common/util/string.h': No such file or directory

I don't think you should try to compile that branch right now, let alone run it. As I mentioned, this was made rather to display certain ideas on class system. "Refactory" branch should be buildable and runnable though.

However, I found I forgot to add new dependancies for the Release version... again
Simply add following to the project's include directories:
$(SolutionDir)\..
or build it as Debug.

[JJS]

Common/core/types.h:
Shouldn't we use the stdint.h names with _t suffix (int32_t instead of int32)? Actually I would say that including stdint.h where supported and only typedefing ourselves on Windows should be preferred.


Configuration file reading:
I think the reading should be split in two parts: one general and one for each platform. For example the sound device selection is platform-specific while the "use speech pack" option is platform independent.


You used a leading underscore for member variables (at least in CEngine). I would be in favour of marking member variables like that instead of not marking them.


void CAGSEngine::winclosehook()
I don't know if this a callback, but it looks like it. If so, it has to be declared static. Otherwise its signature doesn't match the exprected C function prototype. Just saying, I know the code is not at all final.
Fake edit: This is the same situation as with the sound thread entry points, they have to be static I think.


CLIB:
Every time a resource inside the game exe or another archive is opened, this is done through

Code: C

csetlib(archive_file_name, "");
FILE *f = clibfopen("filename.ext", "rb");
fread(); fscanf(); //... whatever
fclose(f);

I wonder if this can be converted to use the stream classes, but I am not sure it can in any case. This is because of the way clib interacts with the Allegro functions which expect a FILE pointer and being able to use stdio functions for access.

[Crimson Wizard]

You used a leading underscore for member variables (at least in CEngine). I would be in favour of marking member variables like that instead of not marking them.

I combined monkey's suggestion and mine own suggestion here: public member variables named CamelCase and have no marking, internal (private and protected) named using "lower" camelCase; leading underscore helps to distinguish one-word member names from local variables, but I added them to all internal vars in sake of consistency.

void CAGSEngine::winclosehook()
I don't know if this a callback, but it looks like it. If so, it has to be declared static. Otherwise its signature doesn't match the exprected C function prototype. Just saying, I know the code is not at all final.

Ah, I actually missed that one.
For other callback, atexit_handler, I did this:

Code: cpp

...
atexit(atexit_handler); // register global function
...

void atexit_handler() {
    CAGSEngine::GetInstance()->atexit_handler();
}

Well, practically the same, just not static member but global function. Don't know which is better; I made Engine a singleton anyway.

[JJS]

I guess the downside of a static member as the callback is that it can itself only access static member variables. But since it is a singleton it doesn't matter really. Either the class instance must be resolved in the static function or in the C helper function - same difference.
The static method has the advantage of not needing that helper function though.

[BigMc]

I just tried to build the refactory branch on Linux and the Makefile is quite outdated again. I saw that you tried to keep it up to date, but that's of course difficult without testing it. Is it possible to arrange the files into folders in such a way that most folders contain only files that are built on all platforms (say all but Common/platform and Engine/platform)? Then all the makefiles would just need a list of folders and a list of files from the platform folder.

[Crimson Wizard]

Is it possible to arrange the files into folders in such a way that most folders contain only files that are built on all platforms (say all but Common/platform and Engine/platform)? Then all the makefiles would just need a list of folders and a list of files from the platform folder.

Not only possible but certainly should be done. I'll check into that, but first I have to finish something. I decided to make a big change at once, but it takes longer than I thought.
I'll post here when it's done.

[BigMc]

The alternative would be to switch to a cross-platform build system like CMake. This will certainly become the best option at some point, but folder based building may also do for now.

EDIT: BTW, http://gitorious.org/ags uses CMake, so their CMakeLists.txt's could be used as a starting point.

[Crimson Wizard]

Alright I am done!
I mean it. Done.
Phew.


The clean-up stage is now "officially" over.
What's left is make sure engine compiles on other supported platforms. Also copy the latest changes from 'main'. Perhaps from 64bit too, but only after it is merged with main somehow.

@BigMC, I've put all platform-dependent files into platform/<platform-name> folders. I believe the rest could be used for any platform. Except the code in obsolete folders- they should not be used at all.

I hope those porting issues will be eventually worked out. Sincerely I got really bored with this code-tidying task and want to move further and do some other interesting things.


//--------------------------------------------------------------------------------------------------
EDIT: some useless statistics (does not include library sources and patches, and unused code):

AGS 3.2.1 SVN:

Common:
- headers: 15
- sources: 12
Engine:
- headers: 4
- sources: 24
Total:
- headers: 19
- sources: 36

Refactory branch:

Common:
- headers: 51
- sources: 39
Compiler:
- headers: 11
- sources: 9
Engine:
- headers: 206
- sources: 220
Total:
- headers: 268
- sources: 268 <--- good coincidence

[Joseph DiPerla]

Awesome work CW! When I get a chance today I will try and compile the source to see if it works for me. Thanks for all your hard work on this!

[BigMc]

@BigMC, I've put all platform-dependent files into platform/<platform-name> folders. I believe the rest could be used for any platform. Except the code in obsolete folders- they should not be used at all.

Great! I commited changes that make it build on Linux again. There were some more Windows and DOS only files and I moved them. It should now be much easier to make it work on the other platforms. Is it something for the coding conventions that all files that are not intended for all platforms should go into "platform"? Also, aren't some of the files currently in the platform folders intended for all platforms and should therefore go somewhere else?

[Crimson Wizard]

Is it something for the coding conventions that all files that are not intended for all platforms should go into "platform"?

Well, maybe. I don't see anything bad in that. There should not be too much of them anyway.

Also, aren't some of the files currently in the platform folders intended for all platforms and should therefore go somewhere else?

I think I was going to put platform-dependent functions and declarations there, like File routines, but now I am not sure. Perhaps function prototypes could be put somewhere else and endian-specific implementation left in platform.
As for agsplatformdriver code, well, it is called 'platform driver' so I guess it belongs there .
I gotta think more about that.

[Joseph DiPerla]

OK, so your new source compiles successfully, but now when I run ACWIN.exe, it crashes. I am using 64-bit windows 7 if that makes a difference, but I do not see how since all other versions of AGS worked before, even the refactored code. Not that this seems very helpful, but here is the error report I get:

Code: AGS

Problem signature:
  Problem Event Name:	APPCRASH
  Application Name:	acwin.exe
  Application Version:	3.2.1.1115
  Application Timestamp:	5002b3d2
  Fault Module Name:	acwin.exe
  Fault Module Version:	3.2.1.1115
  Fault Module Timestamp:	5002b3d2
  Exception Code:	c0000005
  Exception Offset:	00094d16
  OS Version:	6.1.7601.2.1.0.768.3
  Locale ID:	1033
  Additional Information 1:	0a9e
  Additional Information 2:	0a9e372d3b4ad19135b953a78882e789
  Additional Information 3:	0a9e
  Additional Information 4:	0a9e372d3b4ad19135b953a78882e789

Read our privacy statement online:
  http://go.microsoft.com/fwlink/?linkid=104288&clcid=0x0409

If the online privacy statement is not available, please read our privacy statement offline:
  C:\Windows\system32\en-US\erofflps.txt

[Crimson Wizard]

Hmm, runs for me. I too have Win 7 64 bit.
How do you run that exactly?
Do you build and run plain acwin.exe?
Or do you build editor and compile and run some game from editor?
Or else?

I really need more detail.
Also, since you are compiling that, you could make Debug version and run from the MSVS debugger.

[Joseph DiPerla]

I am running ACWIN as is with no game data and not running it through the editor. I was just testing it. Normally what happens when I do that is that I get a long winded message telling me that it needs game data files. Instead, this time, it just crashes on me. Are you still using VC++ 2008 Express or did you upgrade to 2010?

[Crimson Wizard]

I am running ACWIN as is with no game data and not running it through the editor. I was just testing it. Normally what happens when I do that is that I get a long winded message telling me that it needs game data files. Instead, this time, it just crashes on me. Are you still using VC++ 2008 Express or did you upgrade to 2010?

Hmmm... I did the same, and I got that message.
I am using full VC 2008 version, not Express one. Unfortunately I don't remember the differences, I thought they mainly related to mixed code.

Maybe you could run debug version and at least see where it crashes exactly?

[Joseph DiPerla]

First error:

main.cpp at line 426 with END_OF_MAIN(), I get this: Unhandled exception at 0x77e915de in acwin.exe: 0x00000000: The operation completed successfully.

Here is the output from debug:

Code: AGS

'acwin.exe': Loaded 'C:\Joeys fun projects\ags sources\ags-final-eng-refact2\adventuregamestudio-ags-08d4d5f\Solutions\.build\Debug\acwin.exe', Symbols loaded.
'acwin.exe': Loaded 'C:\Windows\SysWOW64\ntdll.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\kernel32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\KernelBase.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\quartz.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\msvcrt.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\gdi32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\user32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\advapi32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\sechost.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\rpcrt4.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\sspicli.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\cryptbase.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\lpk.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\usp10.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\ole32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\oleaut32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\winmm.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\shell32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\shlwapi.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\opengl32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\glu32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\ddraw.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\dciman32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\setupapi.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\cfgmgr32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\devobj.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\dwmapi.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\dinput.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\dsound.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\powrprof.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\imm32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\msctf.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\nvinit.dll'
'acwin.exe': Loaded 'C:\Program Files (x86)\NVIDIA Corporation\coprocmanager\detoured.dll'
'acwin.exe': Loaded 'C:\Program Files (x86)\NVIDIA Corporation\coprocmanager\Nvd3d9wrap.dll'
'acwin.exe': Loaded 'C:\Program Files (x86)\NVIDIA Corporation\coprocmanager\nvdxgiwrap.dll'
First-chance exception at 0x012c4d16 in acwin.exe: 0xC0000005: Access violation writing location 0x00000218.
Unhandled exception at 0x77e915de in acwin.exe: 0xC0000005: Access violation writing location 0x00000218.
First-chance exception at 0x77e8016e in acwin.exe: 0x00000000: The operation completed successfully.
Unhandled exception at 0x77e915de in acwin.exe: 0x00000000: The operation completed successfully.
First-chance exception at 0x77e8016e in acwin.exe: 0x00000000: The operation completed successfully.
Unhandled exception at 0x77e915de in acwin.exe: 0x00000000: The operation completed successfully.
First-chance exception at 0x77e8016e in acwin.exe: 0x00000000: The operation completed successfully.
Unhandled exception at 0x77e915de in acwin.exe: 0x00000000: The operation completed successfully.
First-chance exception at 0x77e8016e in acwin.exe: 0x00000000: The operation completed successfully.
Unhandled exception at 0x77e915de in acwin.exe: 0x00000000: The operation completed successfully.

Here is the output from callstack:

Code: AGS

>	ntdll.dll!77e915de() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for ntdll.dll]	
 	ntdll.dll!77e915de() 	
 	ntdll.dll!77e8014e() 	
 	kernel32.dll!757e7814() 	
 	ntdll.dll!77ee74df() 	
 	ntdll.dll!77ee73bc() 	
 	ntdll.dll!77ed3c48() 	
 	ntdll.dll!77e8016e() 	
 	kernel32.dll!757e7814() 	
 	ntdll.dll!77ee74df() 	
 	ntdll.dll!77ee73bc() 	
 	ntdll.dll!77ed3c48() 	
 	ntdll.dll!77e8016e() 	
 	acwin.exe!_getptd_noexit()  Line 618	C
 	acwin.exe!014aedcf() 	
 	ntdll.dll!77ecb459() 	
 	ntdll.dll!77ecb42b() 	
 	ntdll.dll!77e9edff() 	
 	acwin.exe!_getptd_noexit()  Line 618	C

[Joseph DiPerla]

Also lines 408 with this code in main.cpp gives me an error it seems:

Code: AGS

#ifndef USE_CUSTOM_EXCEPTION_HANDLER
    the_engine->GetSetup()->DisableExceptionHandling = 1;
#endif

[Joseph DiPerla]

Also, when I try to compile release, I get this:


3>libcmt.lib(wincrt0.obj) : error LNK2001: unresolved external symbol _WinMain@16
3>C:\Joeys fun projects\ags sources\ags-final-eng-refact2\adventuregamestudio-ags-08d4d5f\Solutions\\.build\Release\acwin.exe : fatal error LNK1120: 1 unresolved externals

[Crimson Wizard]

Also lines 408 with this code in main.cpp gives me an error it seems:

Code: AGS

#ifndef USE_CUSTOM_EXCEPTION_HANDLER
    the_engine->GetSetup()->DisableExceptionHandling = 1;
#endif

WAAAAIT a minute.
Did I told NOT to run the "pre-advance" branch? It is NOT supposed to work. It simply WON'T.
Use "refactory" branch instead.

[Joseph DiPerla]

Oh crud... I thought I was. Whoops, I was running the project solution in the wrong folder. STUPID FOLDER DISORGANIZATION. My bad!

[Crimson Wizard]

Heh.
Well, I hope it will settle up for you. I must go sleep now so no more tech support today

[Joseph DiPerla]

AWWWWWWWWW....

[Crimson Wizard]

Is it something for the coding conventions that all files that are not intended for all platforms should go into "platform"? Also, aren't some of the files currently in the platform folders intended for all platforms and should therefore go somewhere else?

I changed this a bit. First I moved Common/platform/file to Common/util/file. Then, I made platform/base subfolder and put agsplatformdriver there.
'bigend' header and source files are now in Common/platform/bigend and should not be required by little-end systems. Moved some string-related definitions for Android, IOS and Linux from bigend.h to Common/util/string_utils.h.
Updated Makefile.linux accordingly, but it will be a good idea to test compilation again.

[BigMc]

Ok, good that bigend is now really just for bigendian systems. It will be included on all Unix systems nevertheless, because one does not have distinct makefiles for different endianness.

And the others are handled right that right?

all folders except ((platform and subfolders) except platform/base) -> for all platforms
(platform and subfolders) except platform/base -> depending on platform

EDIT: Since you just moved ifdefed stuff, the question comes to mind if the entire code should be eventually made ifdef-free and everything moved to the platform folders. Probably not before the branch builds on all platforms though.

EDIT2: And ags32bitosdriver has nothing to do with 32 bit. That's 32 bit as opposed to DOS. I mean, since there is no DOS support anymore, the contents should go into other files outside of 'platform'.

[Crimson Wizard]

EDIT2: And ags32bitosdriver has nothing to do with 32 bit. That's 32 bit as opposed to DOS. I mean, since there is no DOS support anymore, the contents should go into other files outside of 'platform'.

I thought it's 32-bit as opposed to 16-bit .
If you think that's not that or should not be emphasized, it should go to platform/base.
On other hand, what if there will be ags64bitosdriver one day?

[BigMc]

I'd vote for suspending the file. It looks like the CD stuff is for Windows and Linux and should therefore go to platform and the rest is for everything and should go somewhere else, don't know where. If there is a need to separate 32bit and 64bit implementations one day, one can do it then with less retarded names than ags32bitosdriver.

[JJS]

I actually don't see the point in AGS32BitOSDriver at all. It only adds two methods to AGSPlatformDriver that are not 32 bit specific. So those methods might as well be pulled into AGSPlatformDriver, then the platform classes can be derieved from that.

[Crimson Wizard]

I actually don't see the point in AGS32BitOSDriver at all. It only adds two methods to AGSPlatformDriver that are not 32 bit specific. So those methods might as well be pulled into AGSPlatformDriver, then the platform classes can be derieved from that.

Did that.

Is it possible to fix other ports on "refactory" branch? So that we could mark the work there completed for now.

[JJS]

Is it possible to fix other ports on "refactory" branch? So that we could mark the work there completed for now.

Is this a pressing issue? Because I cannot really devote the required time right now.

I suppose next on your to-do-list is creating base classes for strings/arrays/and so on?

[monkey0506]

I actually had a question about that. I've seen that it is very common practice for classes like this to provide methods "begin" pointing at the first item in the array and "end" pointing at one past the last item. This means that the pointer returned by "end" is unsafe and can never and must never be dereferenced. Is this really a good practice? I understand the ideology behind it (so you don't have to constantly put "begin() + count()" or what have you), but...I just think that we should generally avoid doing things like this.

[Crimson Wizard]

Is it possible to fix other ports on "refactory" branch? So that we could mark the work there completed for now.

Is this a pressing issue? Because I cannot really devote the required time right now.

I guess it isn't.
Then I will only ask not to add any new features to "refactory" branch. I hope it could stay close to main in terms of execution. So that if something wrong happens we won't have to wonder whether refactored code caused it or a newly added feature.

I actually had a question about that. I've seen that it is very common practice for classes like this to provide methods "begin" pointing at the first item in the array and "end" pointing at one past the last item. This means that the pointer returned by "end" is unsafe and can never and must never be dereferenced. Is this really a good practice? I understand the ideology behind it (so you don't have to constantly put "begin() + count()" or what have you), but...I just think that we should generally avoid doing things like this.

I guess that depends on how we want to use containers in our program. These methods are natural for iterators logic. If we are not going to have iterators they seem completely pointless; we could just have First and Last for containers without random access by element index.

EDIT: on other hand, I don't think it is really obligatory to follow STL-like rules and have dereferencable "end" beyond the last element even if we use iterators (or C#-like enumerators). We may introduce different logic here. STL iterators have other problems, like for certain container types they became invalid as soon as number of elements change.

[Crimson Wizard]

CLIB:
Every time a resource inside the game exe or another archive is opened, this is done through

Code: C

csetlib(archive_file_name, "");
FILE *f = clibfopen("filename.ext", "rb");
fread(); fscanf(); //... whatever
fclose(f);

I wonder if this can be converted to use the stream classes, but I am not sure it can in any case. This is because of the way clib interacts with the Allegro functions which expect a FILE pointer and being able to use stdio functions for access.

At the moment I decided to create CFileStream objects that wrap around returning FILE* pointers from clib. Otherwise we will have to rewrite clib itself. I did not look deeply into this yet, but if my guess is correct it registers some sort of of packed files database?

Also I plan to gradually remove all direct references to clib and use newly introduced CAssetsManager object that returns a file from any data file (provided it exists) something like:

Code: cpp

CFileStream *OpenAsset(const CString &data_file, const CString &file_name, const CString &mode);

That is to keep original way of accessing resources for now.

On other hand, better way would probably be make engine check for files in different locations according to priority rules, like first in the subfolders, then in external data file, then packed in executable. That, however, would require a game setting which allows such behavior, since not every game author would like his game resources be overriden that way.

[JJS]

Game resources are already searched as actual files before accessing the packed files. Check clibfopen(). Data files first/Real files first can be toggled with the cfopenpriority variable.

Also: Yes, clib creates a list of packed files (MultiFileLibNew).


Edit: There might be a problem with returning anything but FILE* variables. I am not sure about it, but it might be that files are closed outside of the engine code (somewhere in Allegro or libraries). So I see a possible memory leak there. It is hard to keep track of this because there are multiple layers of abstraction. A big role plays the overridden pack_fopen Allegro function too. It is modified so that it can take advantage of clib.

[Crimson Wizard]

Edit: There might be a problem with returning anything but FILE* variables. I am not sure about it, but it might be that files are closed outside of the engine code (somewhere in Allegro or libraries). So I see a possible memory leak there.

Hmm, well, from what I've seen I can tell that there's fclose call for each clibfopen right in the engine.

[JJS]

Ok, if you checked this then alright. I am probably wrong about it

[Joseph DiPerla]

Any chance this will be added into the engine ports: https://github.com/duncanc/Lua-for-AGS

[JJS]

Any chance this will be added into the engine ports: https://github.com/duncanc/Lua-for-AGS

I'm on it

[Joseph DiPerla]

Lol. Good to hear.

[Crimson Wizard]

Here's something that confuses me.
There's a comment made for CharacterExtras struct in the engine:

Code: cpp

// UGLY UGLY UGLY!! The CharacterInfo struct size is fixed because it's
// used in the scripts, therefore overflowing stuff has to go here

I cannot figure out why CharacterInfo (or any other struct) should be fixed for scripts?
From what I saw, engine script system stores a pointer to an object (e.g. CharacterInfo), it does not care of its size.
Editor defines the Character "class" contents, but does it really care if the struct in the engine could be larger? What am I missing?

EDIT: ah, actually I do, it seems. Characters array... but how does it know the object size to properly access object by its index? Does the Editor actually calculates its size based on information from agsdefns.sh?

(looks like one of the cases when asking question helps to understand something on your own, still I'd wish I hear from others if I am correct)

[SpeechCenter]

My understanding: yes, the reason is that there is character array in AGS scripts. Although it's not in agsdefns.sh you can see that it's injected to the header in C# Tasks.AppendCharactersToHeader

Code: C#

sb.AppendLine(string.Format("import Character character[{0}];", characters.Count));

and in load_game_file in the engine you have

Code: C++

ccAddExternalSymbol("character",&game.chars[0]);

So, someone could write in the code character[EGO] and that would work. It relies on the size because it's simply defined as an array. There's also some code generated in agsnative that references this array, but I think it's related to old interaction scripts (however, I didn't check this thoroughly).

Another question is whether or not a plugin will depend on the size of this structure. I think it should be fairly safe for most implementations, but need to check this.

[Crimson Wizard]

I really need some advice. For last several weeks I tried to figure out how to work further on the engine, but I did that mostly by inertia. I think I have my usual problem - either I am thinking too hard or too much ahead.
I have a question that needs to be answered and I don't feel like doing that alone, since after all what I do is supposed to be useful to many other people here.

Question is about the aim. What the immediate development should aim for? No, I don't mean implementing cool stuff, I mean the direction of core improvements. Here I have the code I was cleaning-up myself month ago. What next? I myself was going to make a class system, but how it should be done, and what should be the purpose? Should it simply enhance the existing code, mimic it in OOP way? Or should it be brand-new class system written from scratch which aims similar functionality? Or something intermediate? Or else?
The main problem is that I have literally no idea what of things I do will be used (and when). Therefore does gradual re-writing have any sense? If no one is going to use the new code for a long time it might be better to just jump over to the "advanced" stage. Then again, if people want new functionality really badly, maybe it would be better to just stick to cleaned code we have already and implement new things there? On other hand that will require re-implementing more on a later stage.
Maybe I am just not a "planning" person.

[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]

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]

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?

[Crimson Wizard]

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 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.

[Crimson Wizard]

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

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 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'.

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. :/

- 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]

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);

[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.

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.

[Crimson Wizard]

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.

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.
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...

[Calin Leafshade]

There is already a debug console, quake style.

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

[Crimson Wizard]

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.

[Crimson Wizard]

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

[Crimson Wizard]

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

Don't worry, refactored code is practically unused atm anyway, and it will take more time to test it before it may become "official".

[Crimson Wizard]

The console seem to be bugged. It works well with DirectX 5 renderer, but when I run the game with Direct3D 9 renderer and open the console, following eror occurs:

Error: Mismatched colour depths

The game I tested this has 16-bit color depth.

[Crimson Wizard]

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.

I just made the first (and big) step towards this by replacing use of raw allegro BITMAP by IBitmap interface throughout the code (class names may be changed later, that's not a big deal).
This also gave me better understanding of how much Allegro is nested in the engine (...quite).
There's yet much work to be done improving drawing code, it is sprawling over engine and also not very safe.

There's a big problem related to plugins. Plugin interface exposes BITMAP type and return allegro bitmap objects on demand. If there are plugins that use BITMAP directly (that is - by reading/writing returned BITMAP objects), changing plugin interface will break compatibility.
For now I left some workarounds there, but they are hacky and very much unsafe.

The code is in my personal repository, I am waiting for JJS to create new master branch based on refactored code before applying such a big change to the central repo.
https://github.com/ivan-mogilko/ags-refactoring/tree/refactory_bitmap

Meanwhile I was also working on replacing FILE ptrs with stream objects.
https://github.com/ivan-mogilko/ags-refactoring/commits/refactory_file

When all this is done I will (probably) turn towards script interpreter. Don't remember if I mentioned that, but it will be nearly impossible to work with game classes without changing the way script interpreter addresses their members.


PS. Oh, right, and still waiting for (dead)monkey's utility classes, *wink, wink*

[JJS]

I created the master branch based on the current refactory branch. But is also has automatically formatted code and I removed the print_welcome_text() copy protection.

Also in my personal branch here: https://github.com/jjsat/ags/tree/master I added classes for mutex and threads. They are implemented by having a central header that includes the platform specific implementations. Those implementations are descendants of a common base class and are made available to the engine with a typedef. I looked a bit into how to implement this in an efficient way and this seems to be a common approach. Comments are very welcome.

[Crimson Wizard]

I created the master branch based on the current refactory branch. But is also has automatically formatted code and I removed the print_welcome_text() copy protection.

Ouch, erm.... I thought code formatting will be applied a bit later.... I don't know how I will pull my branch there, it will fail to merge.

EDIT: hmm, technically there is a way, but it requires partially reverting commit (only common & engine folders).

[JJS]

Well, I have no problem with deleting the branch again. The formatting changes are automatic anyway and the other change I mentioned is easy to recreate.

E: Or would it merge more easily if you applied the formatting to your branch too?

Code: bash

#!/bin/bash

astyle \
--recursive \
--exclude=Windows/include --exclude=iOS --exclude=scintilla --exclude=Manual --exclude=nativelibs --exclude=buildlibs --exclude=libsrc --exclude=libinclude \
--lineend=linux \
--preserve-date --suffix=none \
--style=allman --add-brackets \
--indent=spaces=2 --convert-tabs --indent-col1-comments --indent-preprocessor \
--align-reference=name \
--pad-oper --pad-header --align-pointer=name --unpad-paren \
--formatted \
*.cpp *.h *.c *.java

[Crimson Wizard]

E: Or would it merge more easily if you applied the formatting to your branch too?

I have doubts on that.
The problem is not that ALL the code is auto-formatted, the problem is that this formatting could change same lines I did and those two variants will conflict with each other: there will be change that make old code formatted and a change that replaces old code with new one (formatted or not). Git won't know which changes to take and this has to be done by hand... all those hungreds of lines I've changed.... I am not going to do that for second time

[JJS]

I deleted the branch.

[Crimson Wizard]

Well, probably reverting one commit was sufficient, but thanks anyway.

I am done with bitmaps for now (that needs heavy testing, but at least I may see them drawn both in engine and editor). I need couple of days more to finish with file streams, then I'll make a branch pull and we may make the auto-formatting.

[BigMc]

Will you do the auto-formatting also for the Editor? In this case, tzachs changes should be merged in first.

[Crimson Wizard]

Will you do the auto-formatting also for the Editor? In this case, tzachs changes should be merged in first.

I strongly doubt that's needed, Editor has nice code already. I think only one C++ file of AGS.Native DLL may benefit from autoformatting, namely: agsnative.cpp.

[Crimson Wizard]

An update.

In my personal branch (for now) I've just finished modifying the script interpreter, letting better control over data it reads and writes. Skipping technical details, the most important consequence is that now it may remap the memory addresses on fly when script wants to read or write object's variables, or even replace direct memory access with calling getter/setter functions if needed (the least has not been implemented yet, because not needed on its own at this very moment).

Simply put: it is now possible to rewrite Character, etc classes and register them as script exports while maintaining full backwards compatibility.
For example, I made a test by adding 100 extra integers to the beginning of Character class and ran a game with calls to player and character[N] objects (both calling functions and accessing variables pre-OO style).

There's a minor concern about speed. I noticed that running game with excessive operations (in rep exec) now adds about 1-4% to CPU (I have 3.4 GHz PC). I have few ideas how to lower that down a bit, but hopefully it won't be a big problem anyway.


There's, however, bigger problem, something I keep forgetting over time. Plugin interfaces. Argh.
AGS exposes a number of structures (character, etc) letting plugins read&write their values directly. Good for speed, but terrrrible for future development. That means that we cannot just get rid of old structures.
For example, assuming that a new class for character is written, it still will have to have the old CharacterInfo structure inside itself, which address will be given to plugins on demand. Same thing goes for all other exposed objects (Room Object, ViewFrame, etc).
I really do not know if there might be a better solution here. Thinking of possible workaround I could only imagine an ugly thing: to give plugins a separate struct with a copy of values and then copy the values back to real class - before and after each event that has been hooked by plugin. But that's probably don't worth it.

[Calin Leafshade]

I think, when considering the impact on the plugin interface, we need to remember that there are very few plugins in active use.

The only one i can think of is the snow/rain plugin which will be arguably obsolete one the engine can blend alpha channels and doesn't access any game data at all.

The Lua plugin would possibly be an issue but that is currently in active development and so it can roll with any changes made to the plugin interface.

[Crimson Wizard]

I think, when considering the impact on the plugin interface, we need to remember that there are very few plugins in active use.

I was hoping that it would be possible to obtain/reimplement sources for all engine plugins that were used for AGS games. If that is achieved we could remake plugin interface in such a way that would fully hide data structures and rebuild all plugins correspondingly. I am not aware how much plugins were actually used. I can only tell - from viewing Modules&Plugins forum - that there weren't billions of them, but may be couple of dozens, which gives me slight hope that may be realistic scenario.

[Calin Leafshade]

The snow/rain plugin was recoded by someone (monkey i think) and released as open source. I think it's in the repos.

All my plugins that have been used (AGSBlend and AGSSpriteFont) were also open sourced and put in the ags repos.

I don't know of any other plugins being used in any ags games.

[BigMc]

Even if the old plugin interface has to stay for compatibility, it may be a good idea to deprecate it and (if a plugin interface is still needed) create a new one in addition.

EDIT: One could also remove the old plugin interface and rewrite the plugins that are needed once someone comes across a game using them.

[Crimson Wizard]

On supporting extended characters in strings (UTF, Unicode). I remember it was quickly discussed back in the summer 2012, and monkey_05_06 made an argument that multibyte strings (in utf-8 encoding) have an advantage over widechar strings in that they are more easily ported. Documentation on C/C++ widechar type tells that it has different size on various platforms: Windows defines wchar_t as 2 byte value, while Linux (and MacOS?) defines it as 4 byte value.

But I was thinking that it is possible to use wide strings internally in the engine, and read/write them as portable multibyte strings in game data / saved games.

Multibyte strings require less memory if only standard latin characters are being used, and probably about same amount of memory if other language/extended characters are used.
On other hand, mb strings may be slower to manipulate with, and require bit more complex algorithms, since their characters have variable widths.

The question is, what type of string is more beneficial in our case?

[Sslaxx]

Neither option strikes me as particularly great, honestly. But extended character support is essential. Which'd be best for cross-platform use (including PSP, iOS and Android) - the varying size of widechar, or handling multibyte characters?

[Snarky]

But I was thinking that it is possible to use wide strings internally in the engine, and read/write them as portable multibyte strings in game data / saved games.

Seems unnecessarily complicated. I don't really know enough to say which option is best, but just on gut feeling I would lean towards multibyte strings.

[BigMc]

UTF-8 is becoming the standard pretty much everywhere. Also it's not like you have to reinvent the code to handle this.

[Crimson Wizard]

But I was thinking that it is possible to use wide strings internally in the engine, and read/write them as portable multibyte strings in game data / saved games.

Seems unnecessarily complicated. I don't really know enough to say which option is best, but just on gut feeling I would lean towards multibyte strings.

That's not really too complicated, but I was probably overthinking this. In case of wide-strings they could always be saved as 16 bit-char arrays. That would cut off some really exotic symbols, but oh well.



Interesting thing is, that Allegro library, which AGS is using, has all the basic string routines made for handling any sort of encoding, while always work with char* arrays. The interpretation of the string depends on initial setting. AGS does it this way:

Code: cpp

set_uformat(U_ASCII);

We may change this to

Code: cpp

set_uformat(U_UTF8);

An investigation should be conducted to see what problems may arise in existing code after such change.

The String class I wrote some time ago may be changed to work with Allegro's string routines instead of stdlib ones.
The biggest problem of UTF8 (varied-size chars), probably, is getting an Nth character. Looking through all chars sequentially may be done with the use of string iterator - this will save time. Getting random character will be much slower, compared to ASCII strings, but I think this operation is rarely used on strings.