Add UrlReference attempt number 2 #564

dfkeenan · 2019-12-07T05:31:32Z

PR Details

This is an attempt to implement #390. Replacing PR #391.

Description

Add new type UrlReference and related engine and Game Studio changes.

Things to-do:

Drag and Drop.
Select an asset button.
Clear reference button.
Select the referenced asset button.
Make collections of UrlReference work.
Using the text box to type the name of assets causes an exception. Bug also exists for existing asset functionality. See Typing the name of an incompatible asset type in a property editor field causes a crash. #393 .
UrlReference being treated as a real asset reference so asset gets included in build and things like renaming assets works the same as for existing asset functionality.
Unit tests.
Documentation. See PR Add UrlReference documentation to the manual. stride-docs#25.

Related Issue

#390

Motivation and Context

See #390.

Types of changes

Docs change / refactoring / dependency upgrade
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My change requires a change to the documentation.
I have added tests to cover my changes.
All new and existing tests passed.

dfkeenan · 2019-12-07T06:29:45Z

For the extension methods I did for UrlReference I used IContentManager. There are a couple of methods on the ContentManager class not on the IContentManager interface that I think would be good to make an extension method for. As far as I can tell there is only one implementation of this interface so I was wondering if it would be ok to change the interface?

The methods I am referring to are:

public T Get<T>(string url)
public bool IsLoaded(string url, bool loadedManuallyOnly = false)
public void Unload(string url)

…nstions.

xen2 · 2019-12-09T14:40:25Z

Thanks for working again on this feature!

It might be good to also upgrade samples. I did a quick git grep of Content.Load (and also noticed a EffectSystem.LoadEffect that might benefit from it).
Note: maybe some of them can't be easily switched, I didn't look in details.

Games/JumpyJet/JumpyJet.Game/PipesScript.cs:            var pipeSetPrefab = Content.Load<Prefab>("Pipe Set");
Graphics/CustomEffect/CustomEffect.Game/CustomEffectRenderer.cs:            customEffect = EffectSystem.LoadEffect("Effect").WaitForResult();
Templates/TopDownRPG/TopDownRPG/TopDownRPG.Game/Gameplay/SceneStreaming.cs:                        Instance = Content.Load<Scene>(Url);
Templates/TopDownRPG/TopDownRPG/TopDownRPG.Game/Gameplay/SceneStreaming.cs:                        var localLoadingTask = loadingTask = Content.LoadAsync<Scene>(Url);
Tutorials/CSharpBeginner/CSharpBeginner/CSharpBeginner.Game/Code/TutorialUI.cs:            tutorialScene = Content.Load<Scene>("Scenes/Basics/" + newTutorialScene.Value);
UI/GameMenu/GameMenu.Game/SplashScript.cs:                SceneSystem.SceneInstance.RootScene = Content.Load<Scene>("MainScene");
UI/UIElementLink/UIElementLink.Game/SplashScript.cs:            SceneSystem.SceneInstance.RootScene = Content.Load<Scene>(NextScene);

dfkeenan · 2019-12-09T23:56:53Z

You are welcome. It has been a bit of a challenge but worth it I think. I have been wanting this feature for quite some time.

I should be able to upgrade the samples some time this week. Might not be until the weekend.

Not sure about EffectSystem.LoadEffect though. I had a quick look it not sure how this would work.

Regarding my proposed changes to IContentManager as stated above. Would this also be acceptable to include?

xen2 · 2019-12-10T23:06:55Z

Seems working well as far as I tested.
Glad you could figure it out -- and you found out a way that didn't require much change, all the better!
I will take a closer look at implementation but this should be good to go soon!

No problem to add some more method in IContentManager.

xen2 · 2019-12-10T23:08:42Z

sources/core/Xenko.Core.Serialization/Serialization/UrlReference.cs

+    [DataStyle(DataStyle.Compact)]
+    [ReferenceSerializer]
+    [DataSerializer(typeof(UrlReferenceDataSerializer))]
+    public class UrlReference


I was wondering if it wouldn't be better as a struct?
Especially since we also have to check for IsEmpty so there is two ways to have invalid values currently (either the UrlReference instance is not set, or IsEmpty is true). That might be little bit annoying to tests those two things everytime.

This would also remove lot of null check as well.
Not sure if all the converters and editor code will like it, but might be worth a try don't you think?

AttachedReferenceManager requires objects because it uses a ConditionalWeakTable to attach references. Ideally the UrlReference types would not have a default constructor so you could not instantiate "empty" ones. But various parts of the asset stuff and UI require a default constructor for them to work. It would require a lot more changes to accomplish this without default constructors.

I have created an interface and a base class so the UrlReference and UrlReference<T> types can both be sealed. I have also tidied up a bunch of things:

Fix some formatting (remove blank likes etc.)

Fixed/added doc comments

Fixed some exceptions being thrown (Had parameters around the wrong way).

Removed/sorted using statements

Added License notice to the tops of files

Good point about attached reference, makes sense. Thanks!

Wondering, could we do it the other way around and forbid to use null/empty string then?
i.e. if not set in editor, UrlReference instance itself is null. If instance is set, it has to have an actual valid Url value?
This implies UrlReference wouldn't have an empty constructor anymore too.

Note: if annoying/complex, no need to do that now.

I did have a couple of solutions that didn't expose the default constructor that worked mostly. For example, make the default constructor internal and change how instantiation code works. I had a couple of different versions of this that "worked" but I wasn't too happy with the bits I changed.

Though I might have just thought of an option....

But even still, doing it this way meant that using collection properties i.e. List<UrlReference> don't work because the UI code cannot instantiate an instance without a default constructor. So would either have to leave this feature out or change the UI code.

That's true, let's keep it like this for now, thanks.

xen2 · 2019-12-10T23:09:54Z

sources/core/Xenko.Core.Serialization/Serialization/UrlReference.cs

+    /// <typeparam name="T">The type off asset.</typeparam>
+    [DataStyle(DataStyle.Compact)]
+    [DataSerializer(typeof(UrlReferenceDataSerializer<>), Mode = DataSerializerGenericMode.GenericArguments)]
+    public sealed class UrlReference<T> : UrlReference


Same goes for this (struct).
Might need a IUrlReference with Url and IsEmpty to act as common base.

Please see reply above.

dfkeenan · 2019-12-11T11:03:56Z

~~I don't think we can use a struct. That wouldn't work with the attached reference stuff. You wouldn't be able to use a struct as a key in ConditionalWeakTable.~~

I have put details in review response instead.

…implement associated methods for UrlReference.

… types allowing sealing of types.

dfkeenan · 2019-12-14T10:16:27Z

As suggested I have updated some of the samples/templates. I have updated:

All projects to 3.2.0.1-beta01
JumpyJet
TopDownRPG
GameMenu
UIElementLink
And the SceneStreaming script template.

Updating the SceneStreaming script template probably means I should also update the "Load scenes" documentation.

(I did not look at the CSharpBeginner tutorials.)

I have noticed some strange behaviour. Using the UrlReference on an asset does make it as referenced and includes it and it's dependencies in the build but they do not all seem to get the Green indicator:

In this example the "Pipe Set" Prefab is referenced as a url and it includes the "Pipe" Prefab. But is not being marked as green.

xen2 · 2019-12-18T11:17:57Z

@dfkeenan this is expected behavior. A prefab is typically design-time only since there's a full copy of it in the scene.
Thanks for the samples update.
Feel free to make the PR non-draft when you believe it's ready for last review/merge.

dfkeenan · 2019-12-18T11:49:02Z

OK. I just never played with Prefabs before I guess. I just tried a small test in 3.1 and it does this. So I didn't break it, or at least not as bad as I thought.

I will change it to ready for review now. But I really should create unit tests.

Have you had a chance to look at the documentation PR? I had a couple of questions in there.

xen2 · 2019-12-20T11:41:02Z

samples/UI/GameMenu/Assets/Shared/SplashScene.xkscene

+                    fd54344b6ac8b2ed232d62dc08820239: !GameMenu.SplashScript,GameMenu.Game
+                        Id: c7c92d4b-2c04-446f-b8c5-59e9d2b989cd
+                        NextSceneUrl: 9294c606-6173-4215-a8dc-0d5043d68333:MainScene
+                    2f704fab49b226562874d765af6b6918: !GameMenu.SplashScript,GameMenu.Game


I suppose it's been added twice as mistake?

Hmm, not sure about that. Hope I haven't introduced some bug.

I do kind of remember having some issues while updating the samples. Game Studio wasn't playing nice when I was changing code. Maybe I tried removing/adding the component while trying to do it. I would just remove it but you all ready merged the change.

xen2 · 2019-12-20T11:42:11Z

Pretty cool how the samples now look, with automatic type inference in the Load and the proper asset id stored in the xkscene!

…tride3d#390) # Conflicts: # samples/Tutorials/CSharpBeginner/CSharpBeginner/CSharpBeginner.Game/CSharpBeginner.Game.csproj # samples/Tutorials/CSharpBeginner/CSharpBeginner/CSharpBeginner.Game/CSharpBeginner.Game.xkpkg

dfkeenan added 3 commits December 1, 2019 02:36

[UrlReference] Attempt 2 initial changes.

fa39ae0

[UrlReference] Fix MemberNode.Update exceptions. And tidy/refactor code.

dfb3c6f

[UrlReference] Fix conversion errors for collections in GraphNodes.

d06dbb6

dfkeenan mentioned this pull request Dec 7, 2019

Add UrlReference documentation to the manual. stride3d/stride-docs#25

Merged

[UrlReference] Improved doc coments on UrlReferenceContentManagerExte…

a43fa4f

…nstions.

xen2 self-assigned this Dec 9, 2019

xen2 self-requested a review December 9, 2019 14:19

xen2 removed their assignment Dec 9, 2019

xen2 reviewed Dec 10, 2019

View reviewed changes

dfkeenan added 3 commits December 11, 2019 21:21

Add more ContentManager methods to the IContentManager interface and …

2024c19

…implement associated methods for UrlReference.

[UrlReference] Created IUrlReference interface and refactored related…

5da1482

… types allowing sealing of types.

[UrlReference] Update samples/templates to use UrlReference.

f00cc0e

dfkeenan marked this pull request as ready for review December 18, 2019 11:49

xen2 reviewed Dec 20, 2019

View reviewed changes

xen2 added 2 commits December 20, 2019 12:43

[Samples] Bumping version

3fabe5f

Merge branch 'master' into UrlReferenceAlt

3a7a54c

xen2 merged commit 5fd1765 into stride3d:master Dec 20, 2019

dfkeenan deleted the UrlReferenceAlt branch December 20, 2019 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UrlReference attempt number 2 #564

Add UrlReference attempt number 2 #564

dfkeenan commented Dec 7, 2019 •

edited

dfkeenan commented Dec 7, 2019

xen2 commented Dec 9, 2019

dfkeenan commented Dec 9, 2019

xen2 commented Dec 10, 2019

xen2 Dec 10, 2019 •

edited

dfkeenan Dec 14, 2019

xen2 Dec 18, 2019 •

edited

dfkeenan Dec 18, 2019

xen2 Dec 20, 2019

xen2 Dec 10, 2019

dfkeenan Dec 14, 2019

dfkeenan commented Dec 11, 2019 •

edited

dfkeenan commented Dec 14, 2019 •

edited

xen2 commented Dec 18, 2019

dfkeenan commented Dec 18, 2019

xen2 Dec 20, 2019

dfkeenan Dec 20, 2019

dfkeenan Dec 20, 2019

xen2 commented Dec 20, 2019 •

edited

Add UrlReference attempt number 2 #564

Add UrlReference attempt number 2 #564

Conversation

dfkeenan commented Dec 7, 2019 • edited

PR Details

Description

Related Issue

Motivation and Context

Types of changes

Checklist

dfkeenan commented Dec 7, 2019

xen2 commented Dec 9, 2019

dfkeenan commented Dec 9, 2019

xen2 commented Dec 10, 2019

xen2 Dec 10, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xen2 Dec 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfkeenan commented Dec 11, 2019 • edited

dfkeenan commented Dec 14, 2019 • edited

xen2 commented Dec 18, 2019

dfkeenan commented Dec 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xen2 commented Dec 20, 2019 • edited

dfkeenan commented Dec 7, 2019 •

edited

xen2 Dec 10, 2019 •

edited

xen2 Dec 18, 2019 •

edited

dfkeenan commented Dec 11, 2019 •

edited

dfkeenan commented Dec 14, 2019 •

edited

xen2 commented Dec 20, 2019 •

edited