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

items: migrate to TRX #2453

Merged
merged 10 commits into from
Feb 7, 2025
Merged

items: migrate to TRX #2453

merged 10 commits into from
Feb 7, 2025

Conversation

lahm86
Copy link
Collaborator

@lahm86 lahm86 commented Feb 6, 2025

Checklist

  • I have read the coding conventions
  • I have added a changelog entry about what my pull request accomplishes, or it is an internal change
  • I have added a readme entry about my new feature or OG bug fix, or it is a different change

Description

I'm going to do an incremental PR for this move, as it's a bit more involved. I'll add a comment with each increment for what to check, then once approved will move to the next stage and re-request review.

This replaces most cases of direct g_Items access with Item_Get.
This replaces msot cases of direct g_Items access with Item_Get.
@lahm86 lahm86 added Internal The invisible stuff TR2 TR1 labels Feb 6, 2025
@lahm86 lahm86 self-assigned this Feb 6, 2025
@lahm86 lahm86 requested review from a team as code owners February 6, 2025 15:03
@lahm86 lahm86 requested review from rr-, walkawayy and aredfan and removed request for a team February 6, 2025 15:03
Copy link

github-actions bot commented Feb 6, 2025

@lahm86
Copy link
Collaborator Author

lahm86 commented Feb 6, 2025

The first stage replaces the majority of g_Items[idx] calls with Item_Get. items.c in both games remains untouched as I want to try to move some common routines that use the array out to TRX, so will do that in following PRs.

The only logical changes at the moment are as follows:

  • adjusting the check for Lara locking on to a new target in TR1
  • simplifying a check for Lara's drawing in TR2 (while she's holding or using the M16)

@lahm86
Copy link
Collaborator Author

lahm86 commented Feb 6, 2025

Second stage removes the global level item count in favour of a getter, and adds a utility for such things as pods and statues to create an item and increment the level count. This count is independent of the actual total item count, as we need to retain it for savegames.

Good things to test in various save/load states (TombATI saves would be good as well to check):

  • the injected PSX mummy in Khamoon
  • pods
  • centaur statues
  • Thor's hammer
  • Skidoo drivers
  • The dragon

@lahm86 lahm86 requested a review from aredfan February 6, 2025 19:16
Copy link
Collaborator

@aredfan aredfan left a comment

Choose a reason for hiding this comment

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

LGTM👍

Copy link
Collaborator

@rr- rr- 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.

const ITEM *const item = Item_Get(g_Lara.weapon_item);
return item->current_anim_state == 0 || item->current_anim_state == 2
|| item->current_anim_state == 4;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a description to what this function does?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, done. Added it to the header. It's sort of an awkward function but as the code was duplicated in four places, I felt it warranted separating out.

This removes duplication for testing if Lara is using the M16 for her
drawing routines.
This provides a common function to retrieve an item index.
This removes the global level item count and makes it private in the
items module. It also adds a convenience function for creating an item
and incrementing this count e.g. for pods and statues.
This moves the next/prev item globals to TRX and makes them private.
This moves item storage to TRX, along with some functions that directly
manipulate the array such as killing and adding active items.
This moves item reading to TRX.
@lahm86
Copy link
Collaborator Author

lahm86 commented Feb 7, 2025

The last three commits above should wrap-up this PR for now. Item storage and reading is now fully in TRX, and I've adjusted object setup and item initialisation to be called commonly from TRX as well, because of the need for this to happen after injections and in the defined order.

A general test here would be good to ensure items activate (enemies, traps, doors etc) and that they are killed. And a sanity check that item state persists over save/load would be good. Thanks.

@lahm86 lahm86 requested review from rr-, aredfan and walkawayy February 7, 2025 10:19
#else
if (item_num < m_LevelItemCount) {
item->flags |= IF_KILLED;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder why we're doing this check in TR2+. I mean, why would the game treat dynamically created objects any different?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it seems a bit odd, but I figured safer for now to keep it as-is. I'll need to study elsewhere to see if the flag is used in different ways possibly first.

Copy link
Collaborator

@rr- rr- left a comment

Choose a reason for hiding this comment

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

It's unfortunate that there are places that need to mess with m_PrevItemActive (and need Item_SetPrevActive) but it is what it is.

This moves the object setup and item initialisation calls to TRX in a
common routine.
This is no longer required for injection processing.
@lahm86
Copy link
Collaborator Author

lahm86 commented Feb 7, 2025

It's unfortunate that there are places that need to mess with m_PrevItemActive (and need Item_SetPrevActive) but it is what it is.

Yes, it is a shame. I believe this is only needed for the body bag feature; I'm not sure if we want to retain that as it seems only required for performance reasons, but that's a separate task to investigate anyway. Would much prefer not to expose this property if we could.

@lahm86 lahm86 merged commit c019b81 into LostArtefacts:develop Feb 7, 2025
8 checks passed
@lahm86 lahm86 deleted the trx-items branch February 7, 2025 14:46
@lahm86 lahm86 mentioned this pull request Feb 7, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal The invisible stuff TR1 TR2
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants