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

[DON'T MERGE YET] Game Events: throw out custom handler_list implementation #1714

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@Vultraz
Member

Vultraz commented May 21, 2017

This replaces it with the standard std::list interface. I've attempted to make this change as least invasive
as possible, so I have not touched the manager::iteration class implementation. It can probably be refactored further later, but the crux of this change is simply to allow the removal of the custom smart_list container while keeping the chance of breakage to a minimum.

The new implementation cannot be an std::forward_list since that only has front-insertion capabilities
while back-insertion is needed.

Game Events: throw out custom handler_list implementation
This replaces it with the standard std::list interface. I've attempted to make this change as least invasive
as possible, so I have not touched the manager::iteration class. It can probably be refactored further later,
but the crux of this change is simply to allow the removal of the custom smart_list container while keeping
the chance of breakage to a minimum.

The new implementation cannot be an std::forward_list since that only has front-insertion capabilities
while back-insertion is needed.
@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz May 21, 2017

Member

tagging @GregoryLundberg for critique

Member

Vultraz commented May 21, 2017

tagging @GregoryLundberg for critique

@Vultraz Vultraz requested a review from Ja-MiT May 21, 2017

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz May 21, 2017

Member

Ok, seems two tests consistently fail with this change... fun!

Member

Vultraz commented May 21, 2017

Ok, seems two tests consistently fail with this change... fun!

@GregoryLundberg

This comment has been minimized.

Show comment
Hide comment
@GregoryLundberg

GregoryLundberg May 21, 2017

Contributor

Even this makes me a bit nervous for a 1.14 (beta 2).

Once you declared we were in the 1.14 beta cycle as opposed to the 1.13.x development cycle, major changes such as this should be locked out. We should be fixing actual bugs. At this stage, these changes seem gratuitous.

Contributor

GregoryLundberg commented May 21, 2017

Even this makes me a bit nervous for a 1.14 (beta 2).

Once you declared we were in the 1.14 beta cycle as opposed to the 1.13.x development cycle, major changes such as this should be locked out. We should be fixing actual bugs. At this stage, these changes seem gratuitous.

@AI0867

This comment has been minimized.

Show comment
Hide comment
@AI0867

AI0867 May 21, 2017

Member

The failure is unsurprising. handler_list::iterator::operator*() and lock_ptr do very different things.

Member

AI0867 commented May 21, 2017

The failure is unsurprising. handler_list::iterator::operator*() and lock_ptr do very different things.

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz May 21, 2017

Member

@AI0867 I delegated the erasure of invalid elements to the iteration ctor.

Member

Vultraz commented May 21, 2017

@AI0867 I delegated the erasure of invalid elements to the iteration ctor.

@AI0867

This comment has been minimized.

Show comment
Hide comment
@AI0867

AI0867 May 21, 2017

Member

@Vultraz That might work if the list (or rather, the set of things pointed to by the list) is guaranteed not to be modified during an iteration. Comments from the old list show that this is almost certainly not the case.

Member

AI0867 commented May 21, 2017

@Vultraz That might work if the list (or rather, the set of things pointed to by the list) is guaranteed not to be modified during an iteration. Comments from the old list show that this is almost certainly not the case.

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz May 21, 2017

Member

@AI0867 actually, it seems the problem that the internal index_ variables in the handlers are no longer contiguous after cleanup.

Member

Vultraz commented May 21, 2017

@AI0867 actually, it seems the problem that the internal index_ variables in the handlers are no longer contiguous after cleanup.

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz May 21, 2017

Member

I'm pondering if I can eliminate the index thing altogether.

Member

Vultraz commented May 21, 2017

I'm pondering if I can eliminate the index thing altogether.

@Vultraz Vultraz changed the title from Game Events: throw out custom handler_list implementation to [DON'T MERGE YET] Game Events: throw out custom handler_list implementation May 21, 2017

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz May 21, 2017

Member

Ok, after further pondering I have found the issue goes deeper than I expected and will require more pondering:

event_handlers owns a vector of event_handlers. However, when an event handler is disabled, it's not actually removed from that vector. Instead, the ptr is reset and it points to a nullptr. This keeps the indices stored in each event_handler correct, since their position never changes.

Under the old (custom) implementation of handler_list, entries were likewise only 'flagged' for removal from the list and never actually removed. By changing it so each one was removed, the 'next' element the list then basically 'skips' an entry in the main vector, so, in the case of the tests, the indices are off by one.

And looking further, it looks like the vector or handlers persists for the lifetime of the manager.

So, I really have two solutions here:

  • Leave the cleanup code commented out and just re-add the validation check for validity when locking the weak_ptr. With the current design, this should "just work", I think.
  • Do the proper thing and remove index reliance and actually clear defunct entries from the main handler vector (though if I do it would probably become a list of its own since constantly removing stuff from within a vector would be rather costly).
Member

Vultraz commented May 21, 2017

Ok, after further pondering I have found the issue goes deeper than I expected and will require more pondering:

event_handlers owns a vector of event_handlers. However, when an event handler is disabled, it's not actually removed from that vector. Instead, the ptr is reset and it points to a nullptr. This keeps the indices stored in each event_handler correct, since their position never changes.

Under the old (custom) implementation of handler_list, entries were likewise only 'flagged' for removal from the list and never actually removed. By changing it so each one was removed, the 'next' element the list then basically 'skips' an entry in the main vector, so, in the case of the tests, the indices are off by one.

And looking further, it looks like the vector or handlers persists for the lifetime of the manager.

So, I really have two solutions here:

  • Leave the cleanup code commented out and just re-add the validation check for validity when locking the weak_ptr. With the current design, this should "just work", I think.
  • Do the proper thing and remove index reliance and actually clear defunct entries from the main handler vector (though if I do it would probably become a list of its own since constantly removing stuff from within a vector would be rather costly).
@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz May 21, 2017

Member

@GregoryLundberg would appreciate it if you commented on the substance of the change not whether it should be merged. Since we still have at least 1 major feature we're hoping to try to add before the next release, it wouldn't be without question to merge this but I need to ensure it's done right.

Member

Vultraz commented May 21, 2017

@GregoryLundberg would appreciate it if you commented on the substance of the change not whether it should be merged. Since we still have at least 1 major feature we're hoping to try to add before the next release, it wouldn't be without question to merge this but I need to ensure it's done right.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel May 21, 2017

Member

No, it should be without question that this is not merged before the next release. We're at the beta stage. Big refactors and big new features should be avoided as much as possible.

What is this one major feature you have in mind, anyway?

Member

CelticMinstrel commented May 21, 2017

No, it should be without question that this is not merged before the next release. We're at the beta stage. Big refactors and big new features should be avoided as much as possible.

What is this one major feature you have in mind, anyway?

@Vultraz Vultraz self-assigned this Jul 26, 2017

@Vultraz Vultraz added this to the 1.15 milestone Jul 26, 2017

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Nov 28, 2017

Member

Closing in favor of #2250.

Member

Vultraz commented Nov 28, 2017

Closing in favor of #2250.

@Vultraz Vultraz closed this Nov 28, 2017

@Vultraz Vultraz deleted the event_handler_list_refactor branch Nov 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment