Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DIRECTOR: Manage filmloop composed of other filmloops. #5643

Merged
merged 1 commit into from Feb 12, 2024

Conversation

kartiksharmakk
Copy link
Contributor

This patch adds support for filmloops composed of other filmloops. It also fixes a bug in the filmloop bbox computation during Sprite::setCast() , we have to create a case kCastFilmLoop to prevent it from going to default case as the default case equates it to dims which is initialrect (0 value) , so the setCast function basically made film loop bbox 0.

trello link:- https://trello.com/c/uOzokEHj/712-fish-in-aquarium-is-not-displayed

Copy link
Member

@sev- sev- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I have a few minor notes, mostly regarding code formatting.

Also, instead of the trello link, please copy over the steps to reproduce from the card, to the commit log message.

subFilmLoops = false;
}
}
else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is badly formatted.

} else {

case kCastShape:
case kCastText: // fall-through
break;
default:
warning("Sprite::setCast(): Setting bbox of castId %s , type: %s to 0", memberID.asString().c_str(), castType2str(_cast->_type));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this still be as a warning(), or is it better to hide it behind a debug(3, kDebugImages. ...?

@@ -110,7 +110,7 @@ Cast::~Cast() {

CastMember *Cast::getCastMember(int castId, bool load) {
CastMember *result = nullptr;

CastMember *subfilmloop = nullptr;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This declaration seems to me very early, You are not using this variable beyond the if()

@@ -120,6 +120,12 @@ CastMember *Cast::getCastMember(int castId, bool load) {
_loadMutex = false;
result->load();
while (!_loadQueue.empty()) {
// prevents double loading of a filmloop of filmloop
if (_loadQueue.back()->_type == kCastFilmLoop) {
subfilmloop = _loadQueue.back();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CastMember *subfilmloop = _loadQueue.back();

@kartiksharmakk kartiksharmakk force-pushed the filmloop_patch branch 3 times, most recently from 5f9c554 to fe60d7e Compare February 11, 2024 17:15
This patch adds support for filmloops composed of other filmloops.
It also fixes a bug in the filmloop bbox computation during Sprite::setCast() , we have to create a case kCastFilmLoop to prevent it from going to default case as the default case equates it to dims which is initialrect (0 value) , so the setCast function basically made film loop bbox 0.

Steps to reproduce:

Run the command: ./scummvm --start-movie="ATD\HD\ccTWRRAD.DXR" totaldistortion-win.
After running the command, the fish will now be visible. Previously, it was not being displayed due to lack of nested film loop support.
@sev-
Copy link
Member

sev- commented Feb 12, 2024

Thank you!

@sev- sev- merged commit 21c0853 into scummvm:master Feb 12, 2024
8 checks passed
@kartiksharmakk kartiksharmakk deleted the filmloop_patch branch February 12, 2024 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants