Skip to content

Big'N'Veiny pickups #493

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

Merged
merged 2 commits into from Jun 29, 2018
Merged

Big'N'Veiny pickups #493

merged 2 commits into from Jun 29, 2018

Conversation

ghost
Copy link

@ghost ghost commented Jun 1, 2018

@ghost ghost changed the title Big'N'Veiny pickups Big'n'Veiny pickups Jun 1, 2018
@ghost ghost mentioned this pull request Jun 5, 2018
7 tasks
@JayFoxRox
Copy link
Collaborator

JayFoxRox commented Jun 19, 2018

This has not gotten any review the past 2+ weeks. Can you provide more information or even a screenshot to attract reviewers?

I assume this can be tested by starting the http://gta.wikia.com/wiki/Big%27n%27Veiny mission.

Some possible things you could have included in the first post:

  • What's still missing to play the entire Big'n'Veiny mission?
  • How was this implemented? I assume the addresses locations are dumped from gta3.exe?
  • Is there anything missing from this PR for the pickups?
  • Is there any specific behaviour which should be tested?

@@ -81,6 +85,34 @@ class PickupObject : public GameObject {
return false;
}

virtual bool isBigNVeinyPickup() const {
return false;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: This does look slightly out of place.
Does GTA3 really do it this way? I'd have expected some kind of enum to identify the pickups by type, instead of such a very specific getter which is only useful for one single item type, used in a single mission of the game.

Copy link
Author

@ghost ghost Jun 19, 2018

Choose a reason for hiding this comment

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

GTAIII does this very poorly, there is separate global pool for these pickups, and separate update, render functions, and special creation and removal functions for opcodes.

Copy link
Author

Choose a reason for hiding this comment

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

There is no, like, actual pickup type, which describes what this pickup is, in original game. We already have behavior specific type variable [same as in original game]. GTAIII just uses model index for that. For example if behavior type is PICKUP_ONCE or PICKUP_ONCE_TIMEOUT, etc it checks for model if it's health pickup model then give health. I guess we can add type enum, and ditch all these special classes maybe?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is fine with me then (for now).

I guess we could have something like a PickupPool struct in the future which could be more similar to what GTA3 does.

But I feel this is such a small portion of the project that it's not worth time to redesign anything at this point. The solution is ugly, but it dos work.

@JayFoxRox
Copy link
Collaborator

JayFoxRox commented Jun 19, 2018

I guess you can squash the "Minor fixes" commit into the first commit, or give the commit a better title which is more descriptive. Theoretically most commits could be named "Minor fixes" or "Major fixes" - so avoid such generic names.

I've skimmed over the code and it looks ok. I did not test it yet and I'm waiting for more information until I feel confident to give my final approval.

@ghost
Copy link
Author

ghost commented Jun 19, 2018

What's still missing to play the entire Big'n'Veiny mission?

It's almost playable, mission softlocks in the end, due to missing AI.

How was this implemented? I assume the addresses are dumped from gta3.exe?

Yes

Is there anything missing from this PR for the pickups?

If you are talking about only Big'N'Veiny pickups then I don't think so.

Is there any specific behaviour which should be tested?

There is no special behaviour, except they only can be picked up in vehicle.

I guess you can squash the "Minor fixes" commit into the first commit

Will do

@ghost ghost changed the title Big'n'Veiny pickups Big'N'Veiny pickups Jun 19, 2018
@@ -507,10 +507,13 @@ void RWGame::tick(float dt) {
clockAccumulator -= 1.f;
}

constexpr float timerClockRate = 1.f / 25.f;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't fully understand this change yet (I'm also not aware of the surrounding code).

Where does this / 25.f come from - gta3.exe?
Does it have to do with FPS or what's the story behind it?

also nit for this commit: commit title is too long; should be reworded to not break into commit description

Copy link
Author

@ghost ghost Jun 19, 2018

Choose a reason for hiding this comment

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

Does it have to do with FPS or what's the story behind it?

Basically, yes, it's tied to FPS.

In GTAIII timers are updated every frame and framerate is locked to 30fps [if you have frame limiter on], there is no accumulators or anything. They may go out of sync depending on game's uptime, due to double precision.

bool onPlayerTouch() override;
};

const static std::array<glm::vec3, 106> bigNVeinyPickupsLocations = {
Copy link

@ghost ghost Jun 24, 2018

Choose a reason for hiding this comment

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

nit: Can it be constexpr? It should be doable on almost every distro. (GLM 0.9.8.0 - 2016-09-11)

@danhedron
Copy link
Member

LGTM

husho added 2 commits June 26, 2018 02:39
Fixed: mission timer
Timer was going crazy due to missing 02d9 opcode, update timer 25 times per second

Fixed: mission timer
Don't beep on every timer update
@danhedron danhedron merged commit 4c357ad into rwengine:master Jun 29, 2018
@ghost ghost deleted the pacman branch June 29, 2018 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants