Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wchen-r7, @bcoles: Where do we want to draw the line with screen effects vs. visual effects vs. what is physical?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Screen being pop up boxes, relics displayed in a browser
Physical being cdrom ejection, scada valve movement
Visual being changing an external LED number display
That's how I'd think, but visual vs screen took a little bit of thinking about
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So are we adding another constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for me that means:
SCREEN_EFFECTS
: Something the user can see on the screen that allows them to realize the machine is being exploited.PHYSICAL_EFFECTS
: It involves a moving object.AUDIO_EFFECTS
: A noise or sound that allows the user to realize the machine is being exploited.VISUAL_EFFECT
: Kind of sounds like a sub-category of screen_effects. A little hard to draw the line for me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also add that physical could be a temperature as well. Like a Crock-Pot :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like sinn3r's summary of constants. I think these are solid (note: small edits):
The question remains whether an external LED or LCD falls within
SCREEN_EFFECTS
or a newVISUAL_EFFECTS
category.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, hardware beeps should go in
AUDIO_EFFECTS
. We're trying to be specific and at the same time not confusing, heh.I think I'll change "the device moves" to "the device makes movement," since the implication is that there is movement, not necessarily locomotion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@h00die: I removed that because it potentially puts us in hot water. No pun intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've decided to leave
VISUAL_EFFECTS
out for now. It's a bit too nuanced to be useful right now, IMHO. I think we are in alignment with the current designations.