Thoughts on refactoring and futher improving the engine

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