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

Adds new EventEffect - moves the target agents into specified state #111

Merged

Conversation

curds01
Copy link
Collaborator

@curds01 curds01 commented Feb 4, 2019

This builds on top of #110. This needs rebasing after that PR merges.

The new EventEffect comes with an example and a dedicated README.md discussing the example.


This change is Reviewable

@curds01 curds01 force-pushed the PR_add_change_state_effect branch from dff5a62 to a25b7e7 Compare February 5, 2019 05:28
@curds01 curds01 changed the title [DoNotMerge] - Adds new EventEffect - moves the target agents into specified state Adds new EventEffect - moves the target agents into specified state Feb 5, 2019
@curds01 curds01 force-pushed the PR_add_change_state_effect branch from a25b7e7 to 1da07b5 Compare February 5, 2019 05:33
Copy link
Owner

@MengeCrowdSim MengeCrowdSim left a comment

Choose a reason for hiding this comment

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

Minor nits

Reviewed 13 of 13 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @curds01)


src/Menge/MengeCore/Agents/Events/change_state_effect.h, line 22 at r1 (raw file):

/**
  @file   change_state_effect.h
  @brief  The definitin of event effect that moves the target agents to a new state.

BTW typo: definition


src/Menge/MengeCore/Agents/Events/change_state_effect.h, line 83 at r1 (raw file):

protected:
  /** @brief  Implements EventEffectFactory::setFromXML().  */
  bool setFromXML(EventEffect * effect, TiXmlElement * node, 

nit: new style Class*


src/Menge/MengeCore/Agents/Events/change_state_effect.h, line 89 at r1 (raw file):

  EventEffect * instance() const override { return new ChangeStateEffect(); }

  /** @brief	The identifier for the "state" string attribute.  */

nit: tabs

- Adds the new EventEffect
- Adds an example to exercise that feature.
- Updates release notes.
@curds01 curds01 force-pushed the PR_add_change_state_effect branch from 1da07b5 to f5e893a Compare February 5, 2019 05:41
Copy link
Owner

@MengeCrowdSim MengeCrowdSim left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @curds01)

Copy link
Owner

@MengeCrowdSim MengeCrowdSim left a comment

Choose a reason for hiding this comment

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

:LGTM:

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @curds01)

Copy link
Owner

@MengeCrowdSim MengeCrowdSim left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@MengeCrowdSim MengeCrowdSim merged commit f01da35 into MengeCrowdSim:master Feb 5, 2019
@curds01 curds01 deleted the PR_add_change_state_effect branch February 5, 2019 05:44
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.

3 participants