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

Fix #1336 by adding a master kill switch for inputs. #1372

Merged
merged 4 commits into from
Jan 8, 2014
Merged

Conversation

emackey
Copy link
Contributor

@emackey emackey commented Jan 7, 2014

No description provided.

@mramato
Copy link
Contributor

mramato commented Jan 7, 2014

I like the idea of a global toggle, but I have a few comments.

  1. I'm not crazy about the name enableAnyInputs, something simpler like enabled or inputEnabled is better in my opinion. Or something else entirely.
  2. This doesn't actually fix the problem, but just moves it around. From what I can tell, if I have an app where I specifically disable input, generating a flight will actually re-enable input.
  3. By caching and restoring the value to prevent the problem I mention in 2, you simply re-introduce the old problem, which allows for multiple asynchronous functions to stomp on this value.

I'm all for solving this problem, since it's an annoying one, but we need to think through everything to make sure all of our bases are covered.

@emackey
Copy link
Contributor Author

emackey commented Jan 7, 2014

For 2, the docs on the new variable explicitly state that it is for transient events only, things that are temporary in nature. In the "Camera Tutorial" sandcastle example, for example, inputs are still disabled using the old method of setting all the values to false, because that's permanent.

What should its name be?

@emackey
Copy link
Contributor Author

emackey commented Jan 8, 2014

If someone can propose a better name for enableAnyInputs I'll be glad to change it. Ideally the name should help make it clear that this is a temporary kill switch for things like flights and drag-selections, and that "permanent" disabling should be done with the other booleans.

Other than that, this is ready to merge.

@mramato
Copy link
Contributor

mramato commented Jan 8, 2014

I assume you didn't like either of my name suggestions? If this is meant to be for isolated cases, I'm okay with it going into master, but I would prefer some stronger wording in the documentation. Maybe "This flag should only be used in transient" as opposed to "typically" used. I think that's what caused my initial objections (which go away if the flag is exclusively for these special cases). Also, master need to be merged in.

@emackey
Copy link
Contributor Author

emackey commented Jan 8, 2014

The others are all enable*, can we call it enableInputs?

@mramato
Copy link
Contributor

mramato commented Jan 8, 2014

Sure.

@emackey
Copy link
Contributor Author

emackey commented Jan 8, 2014

Done.

@mramato
Copy link
Contributor

mramato commented Jan 8, 2014

While testing this, I found #1374 (which is unrelated and happens in master). I did however find another minor issue that is related.

  1. Start Cesium Viewer
  2. Enter a location in the Geocoder and press enter (camera is disabled as expected)
  3. Halfway through the flight, press the Home button.
  4. Camera should still be disabled for the Home flight but isn't. Moving the camera causes it to shake since you are fighting with the animation.

I can write up another bug if you want, but I thought it might be a simple fix in this area of the code. Thoughts?

@mramato
Copy link
Contributor

mramato commented Jan 8, 2014

What I meant was, I can write up another bug and merge this as-is, if you don't have time to look at the issue. The new behavior is still a big improvement over master.

@emackey
Copy link
Contributor Author

emackey commented Jan 8, 2014

Investigating this further. Looks like multiple flights can be running at the same time, and the most recently added one is the one that "wins" each frame. Really only one flight animation makes sense at a time.

From the code I predict there is also a bug where if you start a long flight, and then start a short flight, at the end of your short flight you might jump to near the end of the longer flight.

@mramato
Copy link
Contributor

mramato commented Jan 8, 2014

So this is probably a different manifestation of #377?

@mramato
Copy link
Contributor

mramato commented Jan 8, 2014

Okay, after talking offline with @emackey I agree that this is the same TWEEN/singleton/animation issue that's alluded to in #377. Since it's a minor problem and the original problem was far worse, I'm merging this and we will fix the multi-flight issue when we re-work animation.

mramato added a commit that referenced this pull request Jan 8, 2014
Fix #1336 by adding a master kill switch for inputs.
@mramato mramato merged commit 57c35ac into master Jan 8, 2014
@mramato mramato deleted the issue1336 branch January 8, 2014 19:49
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.

2 participants