Opened 13 years ago

Closed 13 years ago

Last modified 13 months ago

#8690 closed patch

Fallback detection patch

Reported by: SF/buddha_ Owned by: sev-
Priority: normal Component: Engine: AGI
Keywords: Cc:
Game:

Description

Makes fallback detection use an encapsulated version of the game descriptions because we'll need dynamic content generation for the game descriptions at least in the AGI engine's fallback detector.

Before this patch you couldn't generate strings on the fly (e.g. read them from a file) and put the into game descriptions because the c-strings there were const char *.

Also modifies all engines to use the encapsulated version of game description when using detectBestMatchingGame and handle its deallocation.

Ticket imported from: #1733764. Ticket imported from: patches/795.

Attachments (4)

fallbackDetector.patch (23.3 KB ) - added by SF/buddha_ 13 years ago.
Fallback detector patch
fallback-alternate.patch (8.0 KB ) - added by fingolfin 13 years ago.
Alternate (incomplete) patch, for common/ only, rest of original patch needs to be adapted
fallback-alternate2.patch (20.1 KB ) - added by fingolfin 13 years ago.
fallback-alternate2.v2.patch (19.0 KB ) - added by SF/buddha_ 13 years ago.
fallback-alternate2.patch but with additional fixes

Download all attachments as: .zip

Change History (13)

by SF/buddha_, 13 years ago

Attachment: fallbackDetector.patch added

Fallback detector patch

comment:1 by SF/buddha_, 13 years ago

Owner: set to sev-

by fingolfin, 13 years ago

Attachment: fallback-alternate.patch added

Alternate (incomplete) patch, for common/ only, rest of original patch needs to be adapted

comment:2 by fingolfin, 13 years ago

I am confused -- why so complicated? This patch has to change lots of code to accomodate for the new EncapsulatedADGameDesc, but could be a lot simpler.

For starters, EncapsulatedADGameDesc can have a destructor. Secondly, it's not necessary to deal with pointers here -- just change detectBestMatchingGame() to return a EncapsulatedADGameDesc, i.e. EncapsulatedADGameDesc detectBestMatchingGame(...) To be able to still known when the fallback detector returns nothing (i.e. situations where right now it returns NULL), we could just check for an empty getGameID-value. Specifically, replace if (fallbackDesc != NULL) { by if (!fallbackDesc.gameid.empty()) {

Next, simply change EncapsulatedADGameDesc to inherit from ADGameDescription:

struct EncapsulatedADGameDesc : ADGameDescription { Common::String gameid; Common::String extra;

EncapsulatedADGameDesc() { } EncapsulatedADGameDesc(const ADGameDescription &realDesc, Common::String paramGameID = Common::String(""), Common::String paramExtra = Common::String("")) : ADGameDescription(realDesc), gameid(paramGameID), extra(paramExtra) { }

// Functions for getting the correct gameid and extra values from the struct const char *getGameID() const { return gameid.empty() ? ((ADGameDescription *)this)->gameid : gameid.c_str(); } const char *getExtra() const { return extra.empty() ? ((ADGameDescription *)this)->extra : extra.c_str(); } };

(note that an alternative to getGameID/getExtra would be to just set the new gameid/extra field to the value of the "hidden" original fields in the constructor).

Doing all this will remove the need for many of those "delete"s and IMO simplify the code a lot. Attached is a partial patch, for common/ only, which demonstrates my suggestion. File Added: fallback-alternate.patch

comment:3 by fingolfin, 13 years ago

Alternatively, if you prefer using a pointer to a "realDesc" member, (it might make some things easier, like modifying some of the current ::initGame methods), one should still return "EncapsulatedADGameDesc" instances directly, not pointers to them. In that case, one would replace if (fallbackDesc != NULL) { by if (fallbackDesc.realDesc != NULL) {

This approach would still have the advantage of not requiring added "delete" invocations (which are error prone and ugly).

by fingolfin, 13 years ago

Attachment: fallback-alternate2.patch added

comment:4 by fingolfin, 13 years ago

Here is a second alternate patch, based on my second suggestion. It's somewhat shorter than the original patch and requires only minimal changes to each engine.

In addition, I reverted some "0 -> NULL" changes. In C++, NULL is identical to 0 anyway, so these changes were purely cosmetical. We can discuss unifying our 0 vs. NULL usage, but I think it should be done through the backdoor via this patch ;-) File Added: fallback-alternate2.patch

by SF/buddha_, 13 years ago

fallback-alternate2.patch but with additional fixes

comment:5 by SF/buddha_, 13 years ago

Here's the second alternate patch but with some additional fixes.

I couldn't agree more about not wanting additional delete statements because of their error proneness. I just somehow didn't think about this alternate way before. So I definitely prefer not having the deletes...

The changes to the second alternate patch are:

common/advancedDetector.h:

- Fixed testing of realDesc in EncapsulatedADGameDesc struct's getGameID() and getExtra() functions. Was !realDesc when probably should have been realDesc != 0.

- Removed obsolete comments from fallbackDetectFunc's declaration and detectBestMatchingGame's declaration.

common/advancedDetector.cpp:

- detectBestMatchingGame: Fixed possible assertion failure when there are matches, but none has the correct gameid *and* the fallback detector detects something. Removed the assertion too, because it isn't needed anymore.

- detectGameForEngineCreation: Removed !matches.empty() test before calling fallback detector because we would have returned from the function already if there was a match before.

As a sidenote about the first alternate patch:

There might be a problem with casting the EncapsulatedADGameDesc to game specific game descriptions like e.g. AGIGameDescription. I'm not sure how the extra data in the game specific game descriptions would work with it. File Added: fallback-alternate2.v2.patch

comment:6 by fingolfin, 13 years ago

The first alternate patch simply wouldn't have worked with our "fake" ADGameDesc subclasses, something I didn't consider when suggesting it. So that certainly puts a nail into the coffin for that appraoch :-).

comment:7 by sev-, 13 years ago

Status: newclosed

comment:8 by sev-, 13 years ago

Cool. cleaned up couple comments and committed as is. Thank you very much.

comment:9 by digitall, 13 months ago

Component: Engine: AGI
Note: See TracTickets for help on using tickets.