Messages

[also]

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.

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

AFAIK the game loop cycle always runs the rep_ex_always of each module, in the order they are listed in the editor, with GlobalScript and then the current room's script being the last to run. After every script's rep_ex_always runs, I believe that every script's rep_ex is run, then other events. It would be pretty simple to add Display statements to verify the order, but I'm reasonably certain that rep_ex_always runs in the order you'd expect.

Apparently, this algotithm does not work very well, and some of the modules keep old array definitions.

I rarely trust the incremental build for precisely this reason. Especially when publishing for Steam, if I change anything I've learned to do "Rebuild All", even if The Cat Lady does take several years to build.

[major]

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.

Only in the room scripts. This was meant as a way of keeping everything in one (non-room) script.

If you need to run through a sequence of events that involve the player changing rooms, then what you'd generally do is create a global variable to keep track of what point you're at in the sequence. You can use on_event with the eEventEnterRoomBeforeFadein event to help keep track of this.

If you absolutely want to keep all the code for the sequence in a single script, I'd recommend creating separate functions for each room used in the sequence, and then call those from on_event or rep_ex as needed.

One final tip - you can effectively get an "Enter room (after fade-in)" event by doing:

Code: ags

bool afterFadein = false;

function OnEnterRoomAfterFadeinEvent()
{
  // do your after-fade in stuff here (blocking events are okay too!)
}

function on_event(EventType event, int data)
{
  if (event == eEventEnterRoomBeforeFadein)
  {
    afterFadein = true;
  }
}

function repeatedly_execute()
{
  if ((afterFadein) && (IsInterfaceEnabled()))
  {
    afterFadein = false;
    OnEnterRoomAfterFadeinEvent();
  }
}

I think a "SetSensitivity(x, y)" function would make more sense than a property in that case (similar to SetSize method for GUIs). Still allow setting them separately, but use the two parameter method for setting both at once.

And nice job with the inversion feature! Kind of kitschy, but I can't see it being misused, abused, or causing confusion, so why not?

[CW:]

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.

[does not]

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.

[shared_ptr]

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

[is]

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

[except]

When I said:

"new" engine

I was, of course, referring to replacing the "renderer", as you have said, Snarky (in so far as you mean Allegro as the renderer, which also handles audio, user input, etc.). But the problem is that the engine as it exists today is very much hard-coded around (and thus intrinsically tied to) Allegro 4. I'm not talking about rewriting the entire engine itself from scratch. What I'm saying is that creating a new backend interface (for now, based around Allegro 5, but with generics and abstraction in mind) from scratch is the best starting point to break the engine's ties to Allegro 4 (and eventually, Allegro in general). From that point, what I meant by "reimplementing" things was to pull in the existing engine code except for anything related to Allegro, which would be modified at that point to utilize the new backend interface.

Edit: Actually, reading back over it, I think you (Snarky) pretty much have the right idea. Bear in mind though that the engine isn't going to run on both Allegro 4 and 5 simultaneously, so this particular change can't really be done in an incremental way while preserving the full functionality of the engine at every step (functionality will have to be incrementally restored). Once the engine no longer references Allegro 4, any code that has not yet been updated won't compile. This is why I was saying that the code would have to be pulled back in or "reimplemented" in this engine with the new backend.

New features and scripting cabailities can be and are being added to the current code base; and it is clearly false that the current AGS is not performant enough. None of that is worth splitting the community in half over.

To clarify, I think what is being most strongly suggested here (and I'm in agreement with) is to create a "new" engine from scratch (totally blank code project) to implement the backend features (graphics, input, audio, etc.) that will be needed to get things off the ground. The benefit behind this is replacing the Allegro 4 backend with a cleaner, more abstracted interface to using Allegro 5 (which in turn has better cross-platform support, better Android support, etc., etc.). Once the core backend features are implemented, then existing features will be reimplemented in a cleaner way - this preserves the features of AGS (ease of use, functionality, etc.), but will allow us to clean up the code base in a modular kind of way. It's not about reinventing AGS, but trying to replace Allegro 4 in-place in the current code would be a lot more difficult to accomplish.

Work could continue on the current version of AGS while the new backend is being constructed, and thus any new features would be maintained as they are implemented into the "new" engine. This all will take a lot of time and effort, but it could be done in way that the final transition for users (devs) would be totally seamless.

[AGS]

As much as I've proven myself to be an AGS fanboi over the years, it speaks to the ease of use and power of the engine that AGS has hundreds of active users. Disenfranchising them all for some new ideal of what a modern game engine should be seems wrong to me (especially some meta specification that "someone" will implement). I know that improving AGS in its current state is no simple affair, but I don't think that it's a futile endeavor.

I do think that starting from a fresh code base and reimplementing existing core features could be a viable way of moving forward with this, but the end-goal would be to make it a better version of AGS, not just make some brand new engine that couldn't even be recognized as AGS. I think that for this particular upgrade, the focus should be (almost) exclusively on rebasing the core functionality onto the new engine base, which would rely (for now) on Allegro 5. Staying with Allegro should make it simpler to reimplement the portions that currently rely on Allegro 4 instead of converting them over to something like SDL or a purely generic interface. However, I think that using an intermediary wrapper layer for A5 would be a step toward that generic backend. Once we have the main engine rebuilt on top of the Allegro wrapper layer, it will be simpler to extract the interface we need, and allow other implementations from that point I think - I feel like spending too much time focusing on a generic backend interface could kill production....

[never]

Code: ags

  if (cConrad.x >= cMazio.x) {}
  else if (cConrad.x < cMazio.x) {} // or just use else

The two conditions here are mutually exclusive. If one is true, then the other never can be (at that moment). Only one is true at a time, and it being true means that the other is false. You should never, ever, ever, ever, ever, ever do this. Ever.

If there is any possibility that one condition could change the state of cConrad.x or cMazio.x, then it might make sense to have a separate if-statement. But using else if to check the exact inverse of a condition is never right - that is what else means. If you want to add a note to yourself for added clarity, that is when you should add a comment.

Code: ags

  if (cConrad.x >= cMazio.x) {}
  else {} // this MEANS that cConrad.x < cMazio.x!

I will look into adding a drop-down with a menu button if the user swipes down from near the top of the screen.

As for The Cat Lady, it would be easier to add a D-pad overlay or just replace the control schema, because most Android soft keyboards don't have arrow keys.

[both]

It's been discussed before, but as time goes on it seems increasingly as though porting to Allegro 5 would get AGS where it needs to be as a modern engine.

Allegro 5 is well documented, has better support for modern graphics drivers, has built-in threading support, makes up for a lot of the things AGS has hacked into A4 over the years, has UTF-8 string support, has prebuilt libraries for modern Windows compilers (so we could finally abandon both Visual Studio 2008 and the NoNative solution)...the list goes on.

Porting to Allegro 5 will be no small task. Allegro 4 builds a queue of events which are then processed as the game loop executes. Allegro 5 is instead event-driven, which will require major logical changes to the way the engine is coded. Also, there are many cases where there is no one-to-one conversion, though theoretically everything A4 does A5 can still do.

I have taken a look at starting in on this, but I'm not sure if just diving head-first into this will produce anything meaningful.

Would it be feasible to partition off the engine code into smaller segments, so changes can more easily be reviewed, and others could work on other sections? What might be the best way of breaking this up?

Also, it would probably be useful to use a C++ wrapper layer on top of Allegro 5 for RAII garbage collection and the like. I found one called "ALX", but it doesnt seem to conform to good C++ standards (e.g., it doesn't make use of the default case in switch statements, even when that is the desired result). If needed, it would be relatively simple to write our own wrapper layer.

Thoughts?

[Edit:]

A couple of things...

*) You're drawing the image at (0, 66) in the DrawingSurface. You probably want to draw it at (0, 0). The coordinates to DrawImage are relative to the surface, not the screen.
*) Does it make any difference if you change the transparency? Does it appear if you draw the sprite fully opaque?

Edit: You ninja-posted on me. Anyway, that sounds familiar. I think I reported that same behavior some time ago...

Edit 2: Don't recall if I created a tracker entry for it, but I did create a thread. This is definitely a bug, and I don't know if anyone ever looked into it. (Sorry, I'm posting while working on a school project, so I'm mildly distracted )

although it's a lot bigger task than I originally intended

Creating the multi-platform interface for the editor turned out to be a lot more work than I had originally anticipated too, but CW steered me in the right direction by not letting me take messy shortcuts (even if I promised to clean them up later). What he's said isn't a simple, quick fix, but it's worth doing the right way. And don't worry if it takes time, that's why we're not paying you!

[native interop]

As noted in the linked article:

We have released the complete and up-to-date CoreCLR implementation, which includes RyuJIT, the .NET GC, native interop and many other .NET runtime components.

So it seems that once the port of CoreCLR to Linux and OS X is complete then porting the compiler to .NET could safely be seen as much lower priority. As it stands now, the lack of native interop was the driving factor behind moving the compiler code to .NET, although I wouldn't rule it out as not being worthwhile.

For what it's worth, there are other features that are still higher priority for the 3.4 release, so unless someone is dying to work on it, I'd say we just let it take the back burner until CoreCLR is sufficiently stable in its ports.