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

Allowing default versus overriden controller models #1206

Merged
merged 4 commits into from
Oct 30, 2017
Merged

Allowing default versus overriden controller models #1206

merged 4 commits into from
Oct 30, 2017

Conversation

DerekMa-WP
Copy link

Fix for issue #1174 to allow for default models if the OS's models weren't loaded in addition to allowing for overriding the models.

Derek Mantey added 2 commits October 17, 2017 14:37
… AlwaysUseAlternateModel boolean to allow you to specify a default if the models aren't found/loaded or specify a model that should always be used.
Copy link
Contributor

@keveleigh keveleigh left a comment

Choose a reason for hiding this comment

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

This looks good! I haven't gotten a chance to test it yet, but here are some smaller comments I have so far.

@@ -186,6 +186,7 @@ void Update()
{
if (Application.isEditor)
{
#if UNITY_EDITOR
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? The if (Application.isEditor) call above should prevent this from being called outside the Editor.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, because we're calling on the UnityEditor namespace on the line below.

objectReference: {fileID: 0}
- target: {fileID: 114710303647202208, guid: d29bc40b7f3df26479d6a0aac211c355,
type: 2}
propertyPath: AlwaysUseAlternateModel
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there might be some stale references in here, if you could re-save the scene and update it please, that'd be great!

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, yeah it doesn't look like Unity is removing the old stuff. Cleaning up now.

Copy link
Contributor

@SimonDarksideJ SimonDarksideJ left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

@StephenHodgson
Copy link
Contributor

@keveleigh did you get a chance to look over this again? Thoughts?

@keveleigh
Copy link
Contributor

@StephenHodgson I'll be taking a look today! I definitely think this is a needed change, and I think it works well with the in-editor controller models.

#else
yield break;
yield break;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also have LoadAlternateControllerModel(source); right before the yield break;, that way the alternate models will load in the editor if needed. Right now, nothing will load in the editor.

@keveleigh
Copy link
Contributor

Cool! I'm going to approve this as-is, then I'll merge it into master and update it with my new in-editor code.

@keveleigh keveleigh merged commit 9a3aa59 into microsoft:Dev_Unity_2017.2.0 Oct 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants