Dummy StatBlock in MapIso issue

igorko commented 12 years ago

Migrated from clintbellanger/flare#256

Powers and Hazards assume a live StatBlock pointer.

MapIso.cpp around line 800 we create a dummy StatBlock for map-generated powers (e.g. traps). Once that data reaches the Hazard level, I think it's mainly only used to carry the Accuracy stat.

The issue is that we're not currently deleting the dummy pointer, so it's causing a tiny memory leak. We need to be deleting that right after the powers->activate() call.

Make it so that Hazards aren't absolutely dependent on having a StatBlock. Check the Hazard code and the TakeHit code. Perhaps for Map-generated powers we give Hazard a NULL statblock. Give Hazards a 100% accuracy flag for map events. In the Hazard and TakeHit code, always check to see if statblock is NULL before using it.

stefanbeller commented 12 years ago

Update: The file has been renamed to MapRenderer.cpp and the line is about 1230 now.

piotrrak commented 11 years ago

I've worked bit on it, but never got to finished it. Since it is clearly ownership issue - I've taken "brute force" approach to the problem and added simple RefPtr (which every respectful C++ project has in some form, so why flare wouldn't? :D) . It still needs some testing, cleanups, so I am not pushing it here. Also since recently got very little time for hacking on this, wanted to ensure that we want to go this route...

Pasted diff-stats so we know about what kind of invasive change we are talking:

commit 41707de30fc40c01d69d194d2b84014cf9a63c1d Author: Piotr Rak piotr.rak@gmail.com Date: Thu Dec 27 16:10:15 2012 +0100

Add simple RefPtr.

src/Utils.h | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+)

I am feeling bad taste in my mouth after comiting this one somehow :S.

And mostly mechanical huge commit ala s/stat./stat->/

commit 5cfc50018490c93d87a5324d3ce590a849c617fd Author: Piotr Rak piotr.rak@gmail.com Date: Thu Dec 27 18:20:23 2012 +0100

Use RefPtr for StatBlock.

Fixes #122.

Pretty sane alternative to mine NIH RefPtr would be go for tr1::shared_ptr if it isn't to new. It's been available in gcc-4.1.2 in somewhat usable form, also supported by MSVC++ quite early too (can't remember exact version).

So general question is - do we want duck typing destructors with delete, or pay price of reference counting? TBH: I would go for std::unique_ptr which is much cheaper and we could use in most of places, but it is c++11.

Please let me know, if you think it's worth it, or better to hack-around this one, if we want this fixed for release I'll try to find some time soon.

clintbellanger commented 11 years ago

With the engine being so close to feature-completeness, we have to consider that we have precisely one obvious case for a RefPtr. For now I'm holding out for a more specific solution to this single issue.

piotrrak commented 11 years ago

Sure, that's why I've asked. I'll revisit it sometime in future.

piotrrak commented 11 years ago

697016d LGTM, however it might get little confusing with time in regard to copy-ctrors. Please note that refCounter will be copied too if you do:

StatBlock newstats = new StatBlock(*oldstats); newStats->getRef() == oldstats->getRef();

and I believe it shouldn't be the case. Also little nitpick, about refCounter initialization (I've added inline comment).

clintbellanger commented 11 years ago

With apologies to everyone who's worked on this: I believe I've figured out a correct solution to the root of the problem.

We want map events to use powers (e.g. for traps) but before we weren't treating the map events as persistent "entities" that have a permanent statblock. So every time the trap fired we were creating a StatBlock which would never get deleted.

The fix: Add a StatBlock * to the Map Event class (default null). After loading a map, look through each map event to find ones that need a StatBlock (as in traps). Allocate a new StatBlock for them.

Now we're creating those StatBlocks on map load (one per event that needs one) instead of every time the event activates.

When it's time to unload a map, delete all those non-null StatBlocks pointers.

Advantages:

Responsibility for the StatBlock is clear
Memory management is straight forward
Only allocating a StatBlock per event, not per activation (some events can trigger an infinite number of times)
Only allocating/deallocating between maps, not during live action (performance)
Ability to add more base stats to map event powers (e.g. we could give a specific trap something like armor penetration).

stefanbeller commented 11 years ago

This makes indeed sense to me while reading it the first time.

stefanbeller commented 11 years ago

I implemented the idea @clintbellanger had. However I modified the idea a little. The memory allocation for the statblock belonging to the map events is not done beforehand, but lazily, i.e. when the statblock is needed the first time. This makes the implementation super easy, as we do not need to iterate over the events beforehand and decide whether we need to allocate a statblock.

dorkster commented 10 years ago

The StatBlock leak is still present. It might be because it's late, but I can't see why its not getting deleted when we erase events.

However, I think if event erasing was functioning properly, we'd get a segfault when deleting events. Hazards last an arbitrary amount of time and require a StatBlock pointer. So if the event gets deleted before the hazard is done, that's invalid access.

I have a rough idea of how we can sidestep all of this and add some more flexibility. I propose we allow loading a set of stat files (use the StatBlock load code) that get loaded on map load. These can then be referenced by events that use power. The StatBlocks would be stored in a vector that is a member of the Map class. This has some advantages:

all used StatBlocks get created and destroyed on map load/unload. very simple
more flexibility. Maybe we don't want 1000 accuracy for all traps

flareteam / flare-engine

Dummy StatBlock in MapIso issue #122