-
Notifications
You must be signed in to change notification settings - Fork 41
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
items: migrate to TRX #2453
Conversation
This replaces most cases of direct g_Items access with Item_Get.
This replaces msot cases of direct g_Items access with Item_Get.
Download the built assets for this pull request: |
The first stage replaces the majority of The only logical changes at the moment are as follows:
|
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):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM👍
There was a problem hiding this 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; | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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. |
#else | ||
if (item_num < m_LevelItemCount) { | ||
item->flags |= IF_KILLED; | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
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. |
Checklist
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.