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

Rewrite ownership to use faction_id to avoid pointer crashes #34217

Merged
merged 16 commits into from Sep 29, 2019
Merged

Rewrite ownership to use faction_id to avoid pointer crashes #34217

merged 16 commits into from Sep 29, 2019

Conversation

ghost
Copy link

@ghost ghost commented Sep 24, 2019

Summary

SUMMARY: Bugfixes "Rewrite ownership to use faction_id to avoid pointer crashes"

Purpose of change

Fixes #34209
Fixes #34212

Ownership used pointers to factions, this all seemed fine before dynamic factions.
Now when a dynamic faction is deleted, then everything they ever touched now belongs to a dangling pointer.

This requires a rewrite of ownership to use ids for safety and less crashes ( hopefully )

Describe the solution

rewrite vehicle and item ownership to use faction_id , and edit the associated structure and functions to support that

Describe alternatives you've considered

Other solution was to potentially keep dead factions around forever, but that didnt seem viable.

Additional context

I am not quite happy with this solution, Ive had to rewrite a lot, and going by what ive done recently with factions, I expect lots more bugs. Im operating at the edge of what I know how to do, but the onus is on me to fix what I broke, so this is my best effort at that.
I would appreciate scrutiny on this, ive tested and it seems ok, but I cannot judge from the code alone if this is a suitable rewrite.

Copy link
Contributor

@mlangsdorf mlangsdorf left a comment

Choose a reason for hiding this comment

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

In general, you want to encapsulate all of this stuff. That means set_owner() should have a version that takes a player as an argument and does the right thing, and all of this

if( it.has_owner() ) {
    const faction_id item_fac = it.get_owner();
    if( item_fac != g->u.get_faction()->id ) {

should simply be

if( !it.owned_by_player( g->u, true ) ) {

I didn't go through line by line, but just about about time you changed a function parameter from faction * to faction_id was a time when you should have encapsulated the logic instead.

src/inventory.cpp Outdated Show resolved Hide resolved
src/item.cpp Outdated Show resolved Hide resolved
src/item.h Show resolved Hide resolved
src/item.cpp Outdated Show resolved Hide resolved
@ZhilkinSerg ZhilkinSerg added <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` NPC / Factions NPCs, AI, Speech, Factions, Ownership labels Sep 24, 2019
Copy link
Contributor

@mlangsdorf mlangsdorf left a comment

Choose a reason for hiding this comment

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

Getting better, but you need to encapsulate more. The only time you should dealing with faction->id() is inside the accessor functions or when you only have a faction *.

Also, consistently use Character & instead of player & now that the g->u has a get_fac() function.

src/advanced_inv.cpp Outdated Show resolved Hide resolved
src/condition.cpp Outdated Show resolved Hide resolved
src/item.cpp Show resolved Hide resolved
src/item.h Outdated Show resolved Hide resolved
src/npc.cpp Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Sep 25, 2019

More encapsulation, not tested yet, need to sleep, will test tomorrow.

src/item.cpp Outdated Show resolved Hide resolved
src/item.cpp Outdated Show resolved Hide resolved
src/mapgen.cpp Outdated Show resolved Hide resolved
src/item.cpp Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Sep 26, 2019

Fixed vehicle owner name display.
Tested :

  1. loading old save with old faction data serialized.
  2. spawning dynamic NPCs and killing them and checking owner of their dropped items.
  3. killing followers and checking their dropped items.
  4. going to refugee center, checking their owned items and vehicles.
  5. saving/loading in refugee center.

All seems fine.

@ghost ghost deleted the npc_death_faction_fix branch December 19, 2019 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
<Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` NPC / Factions NPCs, AI, Speech, Factions, Ownership
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Looting a dead npc causes a crash Fake targeting NPC causes segfaults in unowned vehicles
4 participants