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