#8690 closed patch
Fallback detection patch
| Reported by: | SF/buddha_ | Owned by: | sev- |
|---|---|---|---|
| Priority: | normal | Component: | Engine: AGI |
| Version: | 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)
Change History (13)
by , 17 years ago
| Attachment: | fallbackDetector.patch added |
|---|
comment:1 by , 17 years ago
| Owner: | set to |
|---|
by , 17 years ago
| Attachment: | fallback-alternate.patch added |
|---|
Alternate (incomplete) patch, for common/ only, rest of original patch needs to be adapted
comment:2 by , 17 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 , 17 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 , 17 years ago
| Attachment: | fallback-alternate2.patch added |
|---|
comment:4 by , 17 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 , 17 years ago
| Attachment: | fallback-alternate2.v2.patch added |
|---|
fallback-alternate2.patch but with additional fixes
comment:5 by , 17 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 , 17 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 , 17 years ago
| Status: | new → closed |
|---|
comment:8 by , 17 years ago
Cool. cleaned up couple comments and committed as is. Thank you very much.
comment:9 by , 5 years ago
| Component: | → Engine: AGI |
|---|
Fallback detector patch