Skip to content
This repository has been archived by the owner on Jan 18, 2022. It is now read-only.

No more schema copying #953

Merged
merged 16 commits into from
May 29, 2019
Merged

No more schema copying #953

merged 16 commits into from
May 29, 2019

Conversation

paulbalaji
Copy link
Contributor

@paulbalaji paulbalaji commented May 20, 2019 •
edited
Loading

Description

Tests

  • repro steps for UTY-2044 no longer crash Unity

Documentation

  • will check if docs need to be updated
  • changelog

Primary reviewers

If your change will take a long time to review, you can name at most two primary reviewers who are ultimately responsible for reviewing this request. @ mention them.

@improbable-prow-robot improbable-prow-robot added A: core Area: Core GDK A: player-lifecycle Area: Player lifecycle feature module size/XL Denotes a PR that changes 300-599 lines, ignoring generated files. A: playground Area: Playground A: tooling Area: Tooling A: transform-synchronization Area: Transform synchronization feature module size/XXL Denotes a PR that changes 600+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 300-599 lines, ignoring generated files. labels May 20, 2019
@paulbalaji paulbalaji mentioned this pull request May 21, 2019
Merged
@improbable-prow-robot improbable-prow-robot added the A: maintenance Area: Project maintenance or hygiene label May 21, 2019
@paulbalaji paulbalaji changed the title Multiple schema path support + no more schema copying No more schema copying May 22, 2019
jared-improbable
jared-improbable reviewed May 22, 2019
View reviewed changes
workers/unity/Packages/com.improbable.gdk.tools/Common.cs Outdated Show resolved Hide resolved
workers/unity/Packages/com.improbable.gdk.tools/Common.cs Outdated Show resolved Hide resolved
workers/unity/Packages/com.improbable.gdk.tools/GenerateCode.cs Outdated Show resolved Hide resolved
workers/unity/Packages/com.improbable.gdk.tools/GenerateCode.cs Outdated Show resolved Hide resolved
workers/unity/Packages/com.improbable.gdk.tools/LocalLaunch.cs
@@ -204,7 +204,7 @@ public static void LaunchLocalDeployment()
BuildConfig();

var command = Common.SpatialBinary;
var commandArgs = "local launch";
var commandArgs = "local launch --enable_pre_run_check=false";
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 definitely needed?

Copy link
Contributor Author

@paulbalaji paulbalaji May 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

yes because it will otherwise call prepare-for-run under the hood - which will not work if schema is not in the default place (e.g. will break if schema is within the packages)

@improbable-prow-robot improbable-prow-robot added A: build-system Area: Build system feature module A: game-object-creation Area: Gameobject creation feature module A: mobile Area: Mobile integration labels May 23, 2019
jamiebrynes7
jamiebrynes7 approved these changes May 24, 2019
View reviewed changes
Copy link
Contributor

@jamiebrynes7 jamiebrynes7 left a comment

Choose a reason for hiding this comment

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

LGTM % the two comments

workers/unity/Packages/com.improbable.gdk.tools/GdkToolsConfiguration.cs
{
if (!string.IsNullOrEmpty(schemaSourceDir))
{
var fullSchemaSourceDirPath = Path.Combine(UnityProjectRoot, schemaSourceDir);
Copy link
Contributor

Choose a reason for hiding this comment

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

Note this will throw if you put invalid characters in the path.

Maybe check against https://docs.microsoft.com/en-us/dotnet/api/system.io.path.getinvalidpathchars?view=netframework-4.8 and add meaningful errors.

Sidenote - we can probably use this to fix the PATH errors people get with spatial and dotnet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like Path.Combine throws an ArgumentException if any invalid characters are used

so might be good to catch that + add a suitable error

workers/unity/Packages/com.improbable.gdk.tools/GdkToolsConfiguration.cs Outdated
emptySchemaSourceDir = true;
}

if (emptySchemaSourceDir)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just move this into an else block with if (!string.IsNullOrEmpty(schemaSourceDir))

@jamiebrynes7 jamiebrynes7 mentioned this pull request May 24, 2019
Closed
Paul Balaji added 11 commits May 24, 2019 16:26
jamiebrynes7
jamiebrynes7 reviewed May 28, 2019
View reviewed changes
Copy link
Contributor

@jamiebrynes7 jamiebrynes7 left a comment

Choose a reason for hiding this comment

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

Small refactoring suggestion

workers/unity/Packages/com.improbable.gdk.tools/GdkToolsConfiguration.cs Show resolved Hide resolved
workers/unity/Packages/com.improbable.gdk.tools/GdkToolsConfiguration.cs Outdated
{
var schemaSourceDir = SchemaSourceDirs[i];

if (!string.IsNullOrEmpty(schemaSourceDir))
Copy link
Contributor

Choose a reason for hiding this comment

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

Invert the if condition and continue if its null or empty:

if (string.IsNullOrEmpty(schemaSourceDir))
{
	errors.Add(...);
	continue;
}

try 
{
	...
}

...

cheeky refactor
b79bfaa 
Signed-off-by: Paul Balaji <[email protected]>
jessicafalk
jessicafalk approved these changes May 28, 2019
View reviewed changes
workers/unity/Packages/com.improbable.gdk.tools/core-sdk.version
@@ -1 +1 @@
13.7.1-gdk-for-unity
13.7.1-gdk-for-unity
Copy link
Contributor

Choose a reason for hiding this comment

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

was that change intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not intended - but shouldn't make a difference

can make a mental note to remove new line for the next PR if needed

...s/unity/Packages/com.improbable.gdk.transformsynchronization/Components/BufferedTransform.cs
@@ -7,7 +7,7 @@ public struct BufferedTransform : IBufferElementData
{
public Vector3 Position;
public Vector3 Velocity;
public Quaternion Orientation;
public UnityEngine.Quaternion Orientation;
Copy link
Contributor

Choose a reason for hiding this comment

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

is that needed?
(same question for the other two files where you made that change)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, to avoid conflicts with the Quaternion defined in the transform module schema - which now gets generated in the Improbable.Gdk.TransformSynchronization namespace

@paulbalaji paulbalaji merged commit 402f53a into develop May 29, 2019
@paulbalaji paulbalaji deleted the bugfix/uty-2044 branch May 29, 2019 15:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jamiebrynes7 jamiebrynes7 jamiebrynes7 approved these changes

@jessicafalk jessicafalk jessicafalk approved these changes

@zeroZshadow zeroZshadow Awaiting requested review from zeroZshadow

@jared-improbable jared-improbable Awaiting requested review from jared-improbable

Assignees
No one assigned
Labels
A: build-system Area: Build system feature module A: core Area: Core GDK A: game-object-creation Area: Gameobject creation feature module A: maintenance Area: Project maintenance or hygiene A: mobile Area: Mobile integration A: player-lifecycle Area: Player lifecycle feature module A: playground Area: Playground A: tooling Area: Tooling A: transform-synchronization Area: Transform synchronization feature module size/XXL Denotes a PR that changes 600+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@paulbalaji @jared-improbable @jamiebrynes7 @jessicafalk @improbable-prow-robot @zeroZshadow