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

HADESCH: New engine for Hades' Challenge #2541

Merged
merged 1 commit into from Oct 24, 2020
Merged

Conversation

@phcoder
Copy link
Contributor

@phcoder phcoder commented Oct 17, 2020

The game is completable but 3 arcade sequences at the end of Minotaur, Medusa and Troy quests.
Probably full of bugs but I already publish it for consideration

@deepcode-ci-bot
Copy link

@deepcode-ci-bot deepcode-ci-bot bot commented Oct 17, 2020

Congratulations 🎉. DeepCode analyzed your code in 2.001 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@phcoder phcoder force-pushed the phcoder:hadeschsquash branch 3 times, most recently from 53890e7 to 7ffce40 Oct 17, 2020
@henke37
Copy link
Contributor

@henke37 henke37 commented Oct 17, 2020

What's with the AmbiantAnimInternal class?

@phcoder phcoder force-pushed the phcoder:hadeschsquash branch from 7ffce40 to fd71d7c Oct 17, 2020
@phcoder
Copy link
Contributor Author

@phcoder phcoder commented Oct 17, 2020

What's with the AmbiantAnimInternal class?

It's needed to have several AmbiantAnim references referring to the same data

@phcoder phcoder force-pushed the phcoder:hadeschsquash branch from fd71d7c to 2c68a0a Oct 17, 2020
@@ -0,0 +1,357 @@
/* ScummVM - Graphic Adventure Engine
*
* Copyright 2020 Google

This comment has been minimized.

@dreammaster

dreammaster Oct 17, 2020
Member

Congrats on getting to the PR stage. First comment.. double-check your comments. Not sure where the "Copyright 2020 Google" has come from. As far as I'm aware, that shouldn't be part of it.

This comment has been minimized.

@phcoder

phcoder Oct 17, 2020
Author Contributor

I added this as per my employer recommendation this makes it easier for me to contribute

This comment has been minimized.

@koitsu

koitsu Oct 17, 2020

You and/or your employer do not understand how GPLv2 works. If you do work for Google, I suggest you talk to whatever internal team is responsible for licensing compliance -- I am sure they will explain to you how this process works correctly.

This comment has been minimized.

@dreammaster

dreammaster Oct 17, 2020
Member

I concur. It really makes it seem like it's claiming ScummVM belongs to Google. Which is patently false. If you work for Google and worked on this engine during office hours and want to give credit to them, it might be better to put in a separate comment in the engine header file instead

This comment has been minimized.

@sev-

sev- Oct 17, 2020
Member

You could achieve the same if you do it similar to, let's say https://github.com/scummvm/scummvm/blob/master/engines/sword25/sword25.cpp#L23

Where we have the standard ScummVM GPLv2+ header followed by additional copyright.

Another good example is
https://github.com/scummvm/scummvm/blob/master/common/dct.h#L23

This comment has been minimized.

@Mataniko

Mataniko Oct 18, 2020
Contributor

Note that the requirement from Google for new files is as follows:

The above copyright notice or a "project authors" style copyright notice to new files, IF permitted by the project maintainers.

This comment has been minimized.

@sev-

sev- Oct 20, 2020
Member

@phcoder So, what is your resolution on this?

namespace Hadesch {
class AmbientAnimStarter : public EventHandler {
public:
void operator()() override {

This comment has been minimized.

@dreammaster

dreammaster Oct 17, 2020
Member

Double-check that you're properly using tabs rather than spaces. It's hard to tell for sure using the Github web interface, but it looks you're not properly using tab indents. If so, maybe you can run one of the various tools available to correctly format everything to the ScummVM requirements

@phcoder phcoder force-pushed the phcoder:hadeschsquash branch from 2c68a0a to 673d15b Oct 17, 2020
Copy link
Member

@sev- sev- left a comment

Impressive code quality. I added a few notes.

engines/hadesch/detection_tables.h Outdated Show resolved Hide resolved
engines/hadesch/detection.cpp Show resolved Hide resolved
engines/hadesch/detection.cpp Show resolved Hide resolved
engines/hadesch/configure.engine Outdated Show resolved Hide resolved
engines/hadesch/hadesch.cpp Outdated Show resolved Hide resolved
engines/hadesch/hadesch.cpp Show resolved Hide resolved
engines/hadesch/hadesch.cpp Show resolved Hide resolved
engines/hadesch/module.mk Outdated Show resolved Hide resolved
engines/hadesch/hotzone.cpp Outdated Show resolved Hide resolved
engines/hadesch/pod_image.cpp Outdated Show resolved Hide resolved
engines/hadesch/pod_image.cpp Outdated Show resolved Hide resolved
engines/hadesch/rooms/ferry.cpp Outdated Show resolved Hide resolved
@sev-
sev- approved these changes Oct 17, 2020
Copy link
Member

@sev- sev- left a comment

I finished the full review. With such code quality, you may easily continue your work right in the tree.

engines/hadesch/rooms/ferry.cpp Outdated Show resolved Hide resolved
engines/hadesch/rooms/monster/cyclops.cpp Outdated Show resolved Hide resolved

static const char *saveDescs[] = {
"",
"%s in intro scene",

This comment has been minimized.

@sev-

sev- Oct 17, 2020
Member

Is the Russian version any different here?

This comment has been minimized.

@phcoder

phcoder Oct 18, 2020
Author Contributor

No. This doesn't come from game itself. It just adds descriptions in scummvm part. Descriptions inside saves are a different things and are usually empty. I discovered that you can name saves only when doing this scummvm

engines/hadesch/video.cpp Outdated Show resolved Hide resolved
engines/hadesch/video.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@mduggan mduggan left a comment

Mostly small style comments - overall looks really nice!

engines/hadesch/hotzone.cpp Outdated Show resolved Hide resolved
engines/hadesch/rooms/monster/illusion.cpp Outdated Show resolved Hide resolved
engines/hadesch/rooms/monster/illusion.cpp Outdated Show resolved Hide resolved
engines/hadesch/rooms/options.cpp Show resolved Hide resolved
@phcoder phcoder force-pushed the phcoder:hadeschsquash branch from 673d15b to cb9b008 Oct 18, 2020
@phcoder
Copy link
Contributor Author

@phcoder phcoder commented Oct 18, 2020

Pushed new version based on the feedback

@phcoder phcoder force-pushed the phcoder:hadeschsquash branch 2 times, most recently from a7c862a to 93b05ab Oct 19, 2020
@sev-
sev- approved these changes Oct 20, 2020
Copy link
Member

@sev- sev- left a comment

I did a second round of review. The only open question is regarding the headers. Would it be possible to move the copyright right after our standard header?

@@ -0,0 +1,357 @@
/* ScummVM - Graphic Adventure Engine
*
* Copyright 2020 Google

This comment has been minimized.

@sev-

sev- Oct 20, 2020
Member

@phcoder So, what is your resolution on this?

assert(0);
}
int rnd = g_vm->getRnd().getRandomNumberRng(0, sizeof(defaultOutros) / sizeof(defaultOutros[0]) - 1);
debug("rnd = %d", rnd);

This comment has been minimized.

@sev-

sev- Oct 20, 2020
Member

Maybe you could hide it deeper than the debug level 0.

@phcoder phcoder force-pushed the phcoder:hadeschsquash branch 2 times, most recently from 714ad5f to af89537 Oct 23, 2020
@@ -1527,8 +1527,15 @@ sub add_paragraph {
add_person("", "Faalagorn", "Few code improvements");
add_person("", "orangeforest11", "Few engine improvements");
end_persons();
end_section();

This comment has been minimized.

@mgerhardy

mgerhardy Oct 24, 2020
Contributor

Looks like a whitespace error here.

The game is completable but 3 arcade sequences at the end of Minotaur, Medusa and Troy quests.
Probably full of bugs but I already publish it for consideration
@phcoder phcoder force-pushed the phcoder:hadeschsquash branch from af89537 to 1a23530 Oct 24, 2020
@sev-
Copy link
Member

@sev- sev- commented Oct 24, 2020

Merging.

@sev- sev- merged commit 0585927 into scummvm:master Oct 24, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deepcode-ci-bot Well done, no issues found!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.