Porting to Allegro 5

Started by monkey0506, Mon 16/02/2015 19:23:03

Previous topic - Next topic

Lt. Smash

#20
That's exactly my opinion. Nothing entirely new, just make the same only better.
Look in really basic terms, the AGS engine in it's current state is something like this:

Code: ags

A1C1B2A2
B1D3A3C3
D1C2
...

(every letter + number stands for a feature/sub-feature of AGS)

while at best (with a rewrite) it should be like this:
Code: ags

A
    A1, A2, A3
B
    B1, B2
C
    C1, C2, C3, C4
...


You see, the old and the new engine all have the same features. The outputted game and the editor will stay the same. However, if you want to add a new feature or fix a bug in the current version, you have to take into account lots of actually unrelated features and be very cautious to not break anything else.
The new more modular (interfaces + implementations) approach, clearly has all features separated (as far as possible). And then the implementations/sub-features only know about their parent feature. If you know there's a bug in B, you can be sure that nothing in A or C breaks if you fix it. The same applies to adding a new feature to C. Features A and B will be totally untouched.

An example of a feature could be the renderer, the scripting language, the interpreter, the audio part etc.

Allegro 5 would fit perfectly as an implementation of the core-feature layer.

Monsieur OUXX

Quote from: Crimson Wizard on Thu 19/02/2015 20:11:34
Well, my main suggestion would be to make engine more generic by introducing interfaces of... don't know how to call them... subsystems?

Yes, yes, yes. I'm convinced that a successfull "freshening up" of AGS will only succeed if enhancements happen progressively. And the best way to achieve that is to make it possible to "switch" implementations of subsystems one by one, and only if really needed.
 

Calin Leafshade

If you'd start from scratch then why use Allegro? Isn't SDL generally better supported and more actively developed?

monkey0506

@Lt. Smash, yes, that is the general idea of breaking the engine into separate "subsystems" (as CW called it), but that goes beyond the scope of this particular thread I think. We would keep that idea in mind, and the new backend could be seen as the first of many subsystems. Future subsystems wouldn't have to start from a totally empty code project, but the reason this subsystem (essentially) requires it is that Allegro is the main entry point of the entire program. Allegro creates the window, initializes all the graphics drivers (etc.), and handles the lifetime of the program. None of the other (future) subsystems bear this same relation to the engine. Creating those other subsystems might be done from a starting point of a blank code file, but none of them would require recreating the actual program entry point.

In any case, as I said, the focus here is (supposed to be :P) about creating that initial subsystem, to begin abstracting the backend while upgrading to Allegro 5.

@Calin: That was generally the case before Allegro 5 was released, but AFAICT it hasn't really been true since then. And once the interface is abstracted from the engine then it could be made generic and an SDL backend could also be provided. It seems simpler (IMO) to stick to Allegro for now, even though not everything will directly translate from A4 to A5.

Crimson Wizard

#24
Quote from: Calin Leafshade on Fri 20/02/2015 16:11:54
If you'd start from scratch then why use Allegro? Isn't SDL generally better supported and more actively developed?
Do you mean Allegro is not being developed actively enough?
http://alleg.sourceforge.net/
or am I missing something...

The largest difference between Allegro 5 and SDL I know is that Allegro 5 uses floating-point numbers everywhere. Which also let it do "subpixel-antialiasing" thing.

Also, I noticed (from changelogs) that Allegro devs actually use code from SDL sometimes :) (both libs are open source and free)

Alberth

Given the current problems of dropping Allegro 4, I'd think it would be useful to keep a clean separation between AGS and the graphics/input/... libraries, eg by having (probably) several interfaces, as already suggested.
In that situation, which library you actually use is not very relevant. One should be able to switch between Allegro, SDL, or any other library without much effort. (That is the entire point of having interfaces.)

So while picking a library must be done, that choice is imho almost irrelevant compared to making the interface(s) clean.
The question therefore is imho not what library to use, but how to make a clean interface design.

An important first step imho is to make it tangible. Flesh out the interface itself in terms of classes, methods, and functions. Document what the functions should do, and how they co-operate.
Try to refrain from writing implementation code (ie it's an abstract interface at this point, even if tomorrow it would be concrete). Code has a lot of details, and they get in the way of thinking at a higher, big-picture level.

An interface document is tangible, it can be read, commented on, and discussed. It's probably wrong at points (and will evolve), but it gives a concrete reference point to work towards. It also clarifies the scope and project boundaries for all involved.
Not sure about the form of the document. It can be a bunch of text files, a set of Doxygen html pages, a set UML diagrams, or an Office file.

Imho the primary goal of coding an implementation of the interface is checking whether the interface is correct. As a happy side-effect, you also get an implementation of the interface. If the interface is found to be wrong or lacking, step back to the high level picture, rethink the concepts without regard to the code you may have. Fix and document the interface as new reference point, then try again to code it.
Coding is cheap if you know what to code.

Picking a library can help you in checking the interface design, while coding.
If you use a library that you know, it reduces work but has the risk that you import library knowledge into the interface. Using a different library adds work in understanding the library, but it may make you think twice about the interface design, as it is relatively easy to check for yourself whether your known library would also fit in it. A third option would be to use 2 different libraries, and build an implementation against the interface for each library. Lot of work, but it pushes the interface to the limits, you cannot cheat without getting into trouble with the other library. Obviously, 3 or more libraries will push even harder at the cost of more work.

monkey0506

I've been looking through ALX, and I've noticed some things that bother me about that particular implementation as a C++ wrapper to Allegro. Minor things like not using default cases in switch statements and even the sheer amount of code duplication, could probably be overlooked without any major impact. I've realized something slightly more insidious though. The wrapper is passing out false shared_ptrs.

The concept of C++11's shared_ptr is that it is a pointer in charge of maintaining its own lifetime. Ownership can be shared (by copying the shared_ptr), and the underlying object is guaranteed (if used properly) to stay alive until there are no shared_ptrs remaining that reference the object. When the last shared_ptr's destructor is called, the object is deleted (freed). However, axilmar (the author of ALX) is abusing this contract with the Shared class provided. The managed parameter in Shared's constructor is used to cripple the underlying shared_ptr and make it incapable of actually deleting the object it is pointing to.

Even worse, the shared_ptr only works by creating one shared_ptr from the raw pointer and all other shared_ptrs are created as copies of a shared_ptr. ALX is generating new shared_ptrs directly from the raw pointers (as returned by the Allegro functions) - though these are generally the non-deleting crippled shared_ptrs.

I'm mentioning all this here to say that ALX (which I mentioned in my first post) is extremely naive and potentially dangerous.

I'm not sure what the best route would be for attempting to properly encapsulate Allegro's pointers (a primary interesting being to add RAII style garbage collection). It probably wouldn't be efficient enough, but perhaps weak_ptrs could be cached and handed out as shared_ptrs... I dunno. Probably a fair talking point if we're going to use (read as: write) a wrapper layer though (which, FWIW, I think we should, as that's also part of abstracting Allegro out of the engine so that other backends could be provided).

Crimson Wizard

#27
@monkey_05_06
I haven't looked in ALX yet, I just want to make a note: a) since Allegro is written in C, it has its own destructors provided for every library object (e.g. al_destroy_bitmap); b) any "destructor" must be called before the library gets uninitialized (which does not happen automatically, you must call al_uninstall_system() explicitly).
Therefore, if one wants to completely automate the proper deletion of the objects, there should perhaps be an object manager that would release all the existing Allegro objects at once.

The dumb way (something that quickly comes to mind, not necessarily incorrect) is to have a "thin wrappers" over Allegro objects. These wrappers will hold only pointer to related Al object. Object manager could have shared pointers to all wrappers, and tell them to delete their wrapped contents at some point. The rest of the program just passes shared_ptrs of wrappers (not raw Al pointers).

For example:
Spoiler

Code: cpp

class AlDisplay
{
   ALLEGRO_DISPLAY *_alDisplay;

public:
   public AlDisplay([...]);
   <...>
};

typedef std::shared_ptr<AlDisplay> PDisplay; // or whatever

[close]

EDIT: Ok, I checked ALX's Bitmap ou of curiousity; I see that the author takes different approach: he makes the wrapper extend the shared_ptr of Allegro object itself.
Need to think this over...

EDIT2: There is other way that came to my mind: let the object manager count Allegro objects created and uninstall Allegro only when the last shared object is deleted. When the engine asks manager to stop, it sets the flag and listens to object deleters. As soon as there is no more objects left, it calls al_uninstall_system().

Alberth

Quote from: monkey_05_06 on Mon 23/02/2015 03:29:33Even worse, the shared_ptr only works by creating one shared_ptr from the raw pointer and all other shared_ptrs are created as copies of a shared_ptr. ALX is generating new shared_ptrs directly from the raw pointers (as returned by the Allegro functions) - though these are generally the non-deleting crippled shared_ptrs.

C++11 does allow this by means of std::enable_shared_from_this, as explained in http://en.cppreference.com/w/cpp/memory/enable_shared_from_this .
It needs a special base class however.

monkey0506

#29
Alberth, that's true, but Allegro is a C library. None of its classes derive from enable_shared_from_this. So the methods that ALX provides are still breaking the shared_ptr ownership contract.




I have created a class that I feel much better accomplishes what axilmar was trying to pull off. The idea is similar to ALX in that there could be wrapper classes created for the various types of Allegro objects, and these wrapper objects would have shared ownership of the underlying Allegro object (except, of course, ALX doesn't actually provide this shared ownership properly). That is, the actual Allegro objects would remain in memory until the wrapper object (and all copies of it) have been deleted.

Code: cpp
#pragma once
#ifndef SHARED_H
#define SHARED_H

#include <memory>
#include <unordered_map>
#include <utility>

namespace NAMESPACE
{
	template<typename T>
	class Shared : public std::shared_ptr<T>
	{
	private:
		typedef std::weak_ptr<T> weak_ptr;
		typedef std::unordered_map<T*, weak_ptr> unordered_map;

		static unordered_map& GetUnorderedMap()
		{
			static unordered_map map{};
			return map;
		}

		template<typename DELETER>
		static void GetSharedPtr(T *value, shared_ptr &ptr, DELETER deleter)
		{
			weak_ptr &p = GetUnorderedMap()[value]; // will default-construct the weak_ptr if it does not exist
			if (p.expired()) // if weak_ptr was default-constructed (or otherwise expired (probably a reused address))
			{
				ptr.reset(value, deleter); // construct a new shared_ptr, taking ownership of the object
				p = ptr; // store a weak_ptr to the new shared_ptr
			}
			else ptr = p.lock(); // we already had a weak_ptr (that hasn't expired), so "lock" it (giving us a shared_ptr which shares ownership with others using this weak_ptr)
		}

	public:
		Shared()
		{
		}

		template<typename DELETER>
		Shared(T *value, DELETER deleter) : shared_ptr{} // default construct to prevent double-deletion if VALUE is already mapped
		{
			GetSharedPtr(value, *this, deleter);
		}

		Shared(T *value) : shared_ptr{}
		{
			GetSharedPtr(value, *this, std::default_delete<T>{});
		}

		Shared(shared_ptr const &ptr) : shared_ptr{ ptr }
		{
		}

		Shared(shared_ptr &&ptr) : shared_ptr{ std::move(ptr) }
		{
		}
	};
} // namespace NAMESPACE

#endif // SHARED_H


The average look-up/insertion time for an unordered_map is O(1) (constant time), and the worst-case scenario would be O(size()) based on the size of the map itself. These look-ups/insertions would only occur when a Shared object is created, so I imagine that even in the worst-case scenario this would still be very efficient. This would allow classes like the ALX Bitmap class to function properly - take for example Bitmap::getTarget().

ALX's Bitmap::getTarget() method returns the target bitmap for Allegro's drawing functions. ALX has this implemented such that Bitmap::getTarget() returns a Bitmap object that does not own the underlying ALLEGRO_BITMAP*. That is, the bitmap could be deleted or destroyed at any moment, and the Bitmap object would be left totally unaware of this.

Conversely, if the Bitmap class were implemented using the Shared class which I provided above, then Bitmap::getTarget() could return a Bitmap with true shared ownership of the ALLEGRO_BITMAP*. Another Bitmap object going out of scope would not suddenly steal the bitmap away from you. The ALLEGRO_BITMAP* would not be destroyed until the final Bitmap referencing it was destroyed first. This is exactly how the shared_ptr class and RAII idiom are meant to work.

Crimson Wizard

Just to be sure I understand why this map is needed: this is to guarantee that the newly created shared pointer will always have same reference count for any other shared ptr created for same Allegro object?

monkey0506

CW: YES! That is exactly the problem that this class uses the map to overcome. It ensures that when you have a raw pointer returned by Allegro, then construct a shared_ptr from (using the Shared class), it will have true shared ownership with existing shared_ptrs that already own that raw pointer (if any). Consider this example:

Example of the Shared class I provided in my above post - a wrapper for ALLEGRO_MUTEX* (based off ALX):

Code: cpp
#pragma once
#ifndef ALLEGRO_MUTEX_H
#define ALLEGRO_MUTEX_H

#include <allegro5/allegro.h>
#include "Shared.h"

namespace Allegro
{
	class Mutex : public NAMESPACE::Shared<ALLEGRO_MUTEX>
	{
	public:
		Mutex(ALLEGRO_MUTEX *mutex) : Shared{ mutex, al_destroy_mutex }
		{
		}

		Mutex(bool create = true, bool recursive = false) :
			Shared{ create ? (recursive ? al_create_mutex_recursive() : al_create_mutex()) : nullptr, al_destroy_mutex }
		{
		}

		void Lock()
		{
			al_lock_mutex(get());
		}

		void Unlock()
		{
			al_unlock_mutex(get());
		}
	};
} // namespace Allegro

#endif // ALLEGRO_MUTEX_H


The key difference between this and the ALX implementation is that this guarantees that when the Mutex object is constructed, it has shared ownership of the ALLEGRO_MUTEX*. ALX allowed for a Mutex object to be constructed without shared ownership of the ALLEGRO_MUTEX*. This could lead to a dangerous situation where a Mutex object is trying to lock or unlock an already deleted ALLEGRO_MUTEX. Instead, my implementation ensures that other Mutex objects do not free the ALLEGRO_MUTEX* while this Mutex object is trying to use it.

monkey0506

I've been going through ALX, working on cleaning things up, and it seems like it's still a good starting-off point for writing the custom wrapper layer. The String implementation it provides is UTF-8 compatible, automatically converts wchar_t strings, and axilmar has provided overloads for std::hash<String> and the stream operators. I haven't yet come across any other major problems aside from his broken Shared class. All of my tests have shown that the Shared class I provided above is working properly. Creating new objects from the same raw pointer yields a smart pointer with the correct (shared) reference count.

axilmar was also using the Shared class for classes like his File class. IMO, copying (and thus, sharing) a File object doesn't really make sense. If you call the Close method on one instance it would invalidate the other "shared" instances. axilmar tried to work around this with an intermediary class to check that the pointer was only freed once, but that really isn't the point. I changed the File class to use unique_ptr instead, since that makes far more sense than shared ownership of a single file handle.

Crimson Wizard

You could maybe do a pull request to ALX, and then have it as a submodule to your own projects.

Calin Leafshade

If C++ is important for you then why not use a gfx lib with c++ bindings like SDL?

(sorry for pushing SDL, i don't really care either way but it seems like you're keeping allegro for no real reason)

Crimson Wizard

#35
Quote from: Calin Leafshade on Sun 01/03/2015 12:01:07
If C++ is important for you then why not use a gfx lib with c++ bindings like SDL?
Monkey started this thread with introducing existing C++ binding for Allegro... which is being discussed for last few posts too.

Quote from: Calin Leafshade on Sun 01/03/2015 12:01:07
(sorry for pushing SDL, i don't really care either way but it seems like you're keeping allegro for no real reason)
So, we should instead switch to SDL for no real reason?

Calin Leafshade

If its a new interpreter then it's not "switching" to anything.
Just saying you should analyse the options available and not use allegro just because the original AGS uses it.

No hostility, just a suggestion.

Crimson Wizard

#37
Quote from: Calin Leafshade on Sun 01/03/2015 12:21:23
Just saying you should analyse the options available and not use allegro just because the original AGS uses it.

Yes, that's true.
Although something tells me that we won't be able to analyse all the options without spending lots of time. And in the end, all we know is that these libraries can do similar things just little differently... all of them would suit the majority of purposes, and the specific problems will only be known after we will be using them for some time.
:undecided:

I would support what Alberth said about extracting back-end interface: http://www.adventuregamestudio.co.uk/forums/index.php?topic=51751.msg636507931#msg636507931
If that is the aim, then any library would fit as a first try.

E: or we could forget about gfx libraries and write a game engine as a part of the ScummVM.

monkey0506

SDL doesn't have a C++ binding, it is just C++ compatible (as it is written in C). The entire purpose of the compatibility layer is to provide additional benefits of the C++ language (specifically, RAII style garbage collection) that neither Allegro nor SDL have support for as C itself lacks support for these features.

The compatibility layer also serves as a partial abstraction of the backend interface. If there's a better C++ binding for Allegro or SDL that don't have the problems that ALX presents, then I have no qualms about using those, but I haven't seen any. ALX has problems that make it ill-suited to immediate use, but I still feel it is a great jumping off point.

Quote from: Crimson Wizard on Sun 01/03/2015 12:35:21or we could forget about gfx libraries and write a game engine as a part of the ScummVM.

But yet the entire point of this thread is to not rewrite an entirely new engine, but yet to replace the backend that AGS uses.

If anyone else wants to put the work in and show me how easy it would be to take one of these other routes, then by all means do so. Until then, I am manually going through ALX to root out problems it has (naive/broken shared_ptr usage, partial/incomplete implementation of types, poor conformity to C++ standards, etc.) with the goal of establishing a working framework to merge the AGS codebase onto.

monkey0506

Just as an update on my progress...

I discovered something about ALLEGRO_USTR that I really can't agree with, which carried over into the ALX String implementation: a total lack of consistency between code point offsets and byte offsets when accessing a character in the string. Since ALLEGRO_USTR is a UTF-8 string, the characters are variably sized (1 to 4 byte) code points. However, I see no practical reason whatsoever why a string class should allow you to access a byte in the middle of a code point directly. If you absolutely need access to the raw bytes, then the underlying char* can be exposed as needed.

To this end, I completely revamped the String and String::CodePointRef classes. My working version of the String class refers to characters exclusively as String::CodePoint objects which represent the whole character. All offsets are code point offsets, not byte offsets. Overall I feel that this is a much better representation of what people think of when they speak of a String class.

There's also a problem with ALX's Bitmap::LockedRegion class, in that both Bitmap and Bitmap::LockedRegion can be used to unlock the Bitmap (the Allegro functions only permit the ALLEGRO_BITMAP* to be unlocked, not the ALLEGRO_LOCKED_REGION*). ALX's solution to this was to simply make the LockedRegion a non-owner of the Bitmap. The issue with this approach is that ALX's pseudo-shared Shared class had no way of telling if the object had been destroyed - ergo, the LockedRegion would have no way of knowing if the Bitmap had been destroyed. I spent some time looking into other approaches, but ultimately what seemed best was to just make the LockedRegion an owner of the Bitmap that in turn owns the LockedRegion. When the LockedRegion is unlocked (not necessarily destroyed), it releases its ownership of the Bitmap (allowing the Bitmap to be destroyed, if there are no other owners); when the Bitmap is unlocked, it releases its ownership of the LockedRegion (allowing the LockedRegion to be destroyed). This type of circular reference didn't seem particularly ideal, but with a proper Shared class it should work as intended.

This is all rather frustrating, but I feel that it will be worth it in the end...

I've noticed that some of the ALX classes really don't have any purpose anyway, like the EventSource class -- ALLEGRO_EVENT_SOURCE*s are never allocated or deleted by Allegro, so the pseudo-shared implementation ALX provided of that class could never have ownership of the pointer (well, it could allocate/delete a pointer itself, but it doesn't). I've parsed somewhere around half of the ALX files. I think I'll start building the program entry point and see how far I can get with that...

Oh, and Visual Studio is terrible. I'm on the verge of abandoning it because of its horrific implementation of the C++ standard. >:(

SMF spam blocked by CleanTalk