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

NWScript object management #341

Closed
wants to merge 19 commits into from
Closed

NWScript object management #341

wants to merge 19 commits into from

Conversation

seedhartha
Copy link
Contributor

This PR makes NWScript variables store object ids, not object pointers. It introduces two new classes: ObjectReference, which holds the object id, and ObjectManager, which acts as the global registry for objects. This PR also implements destroyObject function in KotOR.

Copy link
Member

@DrMcCoy DrMcCoy left a comment

Choose a reason for hiding this comment

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

There's still use-after-free crashes here: https://gist.github.com/DrMcCoy/0fd8777a1a179bd921de3192895f24d0

The reason seems to be that the _owner and _triggerer member variables in NCSFile are still plain pointers. When assigning those to stack variables, the ObjectReference tries to get their ID, crashing.

@@ -53,11 +56,15 @@ namespace Sonic {

Placeable::Placeable(const Aurora::GFF4Struct &placeable) : Object(kObjectTypePlaceable),
_typeID(0xFFFFFFFF), _appearanceID(0xFFFFFFFF), _scale(1.0f) {
_id = Common::generateIDNumber();
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, Placeable::load() reads the _id out of the GFF, though. There's probably no reason it has to be this _id, though, so you can probably add a new variable, say _placeableID or something, and assign the value from the GFF there.

@@ -47,6 +50,9 @@ using ::Aurora::GFF4List;
using namespace ::Aurora::GFF4FieldNamesEnum;

Object::Object(ObjectType type) : _type(type), _static(true), _usable(false) {
_id = Common::generateIDNumber();
Copy link
Member

Choose a reason for hiding this comment

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

For areas, the _id gets overwritten in Area::loadEnvironment(), with a value from the GFF. You'll need to add a commit adding a, for example _envID value to Area and change the assignment in loadEnvironment to use write into that value instead.

Same applies to Dragon Age II as well.

@DrMcCoy
Copy link
Member

DrMcCoy commented Aug 26, 2018

Otherwise, I like this PR. Looks to me like it goes in the right direction :)

@seedhartha
Copy link
Contributor Author

Fixed the above issues.

@DrMcCoy
Copy link
Member

DrMcCoy commented Aug 27, 2018

Sorry, got another use-after-free here: https://gist.github.com/DrMcCoy/549d6f2ed5ea7532539ee148a02f75a7

The issue here is that Module::_delayedActions contains raw Object pointers (ActionQueue, which is a std::multiset, struct Action members owner and triggerer)

@seedhartha
Copy link
Contributor Author

Done.

@DrMcCoy
Copy link
Member

DrMcCoy commented Aug 28, 2018

I can't seem to trigger any crashes now, yes. :)

However, could you apply bc7047f to the other engines? All except for Sonic have that struct Action with a raw Object pointer in module.h.

@seedhartha
Copy link
Contributor Author

Modified actions/events in other engines as well.

@DrMcCoy
Copy link
Member

DrMcCoy commented Aug 28, 2018

Merged as 08d31ac...61cd69b, thanks! :)

@DrMcCoy DrMcCoy closed this Aug 28, 2018
@seedhartha
Copy link
Contributor Author

Have you intentionally not included the last DA commits or is something wrong with GitHub today?

@DrMcCoy
Copy link
Member

DrMcCoy commented Aug 28, 2018

Huh, yeah, I somehow missed them.

GitHub is weird today, a bit slow with the updates. But this might be completely on me, I think I forgot to update the remote before merging. I'll add them, sorry

@DrMcCoy
Copy link
Member

DrMcCoy commented Aug 28, 2018

There, added them as 61cd69b...6c7e9c6. And made sure the commits I merged are the correct ones and nothing went missing.

@seedhartha seedhartha deleted the objref branch August 29, 2018 00:17
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.

None yet

3 participants