Opened 12 years ago

Closed 12 years ago

Last modified 7 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_ 12 years ago.
Fallback detector patch
fallback-alternate.patch (8.0 KB) - added by fingolfin 12 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 12 years ago.
fallback-alternate2.v2.patch (19.0 KB) - added by SF/buddha_ 12 years ago.
fallback-alternate2.patch but with additional fixes

Download all attachments as: .zip

Change History (13)

Changed 12 years ago by SF/buddha_

Attachment: fallbackDetector.patch added

Fallback detector patch

comment:1 Changed 12 years ago by SF/buddha_

Owner: set to sev-

Changed 12 years ago by fingolfin

Attachment: fallback-alternate.patch added

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

comment:2 Changed 12 years ago by fingolfin

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 Changed 12 years ago by fingolfin

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

Changed 12 years ago by fingolfin

Attachment: fallback-alternate2.patch added

comment:4 Changed 12 years ago by fingolfin

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

Changed 12 years ago by SF/buddha_

fallback-alternate2.patch but with additional fixes

comment:5 Changed 12 years ago by SF/buddha_

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 Changed 12 years ago by fingolfin

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 Changed 12 years ago by sev-

Status: newclosed

comment:8 Changed 12 years ago by sev-

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

comment:9 Changed 7 months ago by digitall

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