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

Add UrlReference #391

Closed
wants to merge 21 commits into from
Closed

Add UrlReference #391

wants to merge 21 commits into from

Conversation

dfkeenan
Copy link
Contributor

@dfkeenan dfkeenan commented Feb 23, 2019

PR Details

This is an attempt to implement #390.

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. May need to change AddNewItemCommand. Or making it so UrlReference have a default constructor, which I will do if we end-up revmoving AssetId from it.
  • 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.

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 dfkeenan changed the title Start work on UrlReference Add UrlReference Feb 23, 2019
@dfkeenan
Copy link
Contributor Author

dfkeenan commented Feb 23, 2019

I have made some progress. There are still a number of issues with the implementation though.

  1. The "Select an asset" and "Select the referenced asset" buttons don't work.
  2. The asset is not marked as referenced.
  3. Drag and Drop doesn't work.
  4. When using the textbox to enter url the UrlReferenceToUrl.ConvertBack method can cause an exception if the the property is of type UrlReference<T> and the asset type are not compatible. I am unsure about how to get the type of the target property. The value of the targetType parameter is object.

@dfkeenan
Copy link
Contributor Author

dfkeenan commented Feb 24, 2019

I have made some progress. Feature implementation-wise there are still a couple of outstanding issues:

  • UrlReference is not treated as an asset reference and getting all associated functionality. i.e. Make asset be included in build, renaming/deleting asset does not update the UrlReference. I have not looked into how this works yet. Some pointers would be nice.
  • I am not happy with the UrlReference types. I would like the properties to be internal or at least their setters. I am not sure how to make serialization handle this. Do I need to write a custom serializer? I have created custom serializers.
  • I don't really like the name UrlReference I think something like ContentUrlReference might be better.

@xen2 and @Kryptos-FR , Some feed back and help about my above questions would be greatly appreciated.

@dfkeenan
Copy link
Contributor Author

I guess the appropriate place for documentation in the manual would be on this page: https://github.com/xenko3d/xenko-docs/blob/master/en/manual/game-studio/use-assets.md

@dfkeenan
Copy link
Contributor Author

There has been some disagreement between @xen2 , @Kryptos-FR and myself regarding the design of this feature. Mostly about the use of/including AssetId in UrlReference. So I am not sure how to progress from here.

@xen2
Copy link
Member

xen2 commented Feb 26, 2019

As discussed in the chat, I was thinking the way to go is probably to:

  • Include only a string in UrlReference
  • Attach an AttachedReference to the UrlReference at design time (using AttachedReferenceManager)

That way, runtime object contains only string but editor has all info.
Also, it will likely be able to reuse most of the existing code path.
I will double check if there is no blocker tomorrow.

@dfkeenan
Copy link
Contributor Author

dfkeenan commented Mar 9, 2019

Putting this on hold until @xen2 finishes the discussed refactor/re-architecture of AttachedReference, AttatchedReferenceManager etc.

@dfkeenan
Copy link
Contributor Author

dfkeenan commented Jul 2, 2019

This branch is getting a fair bit behind master now. I wonder if it's worth updating.

@profan
Copy link
Contributor

profan commented Aug 14, 2019

What's the status on this? 👀

@dfkeenan
Copy link
Contributor Author

I have not looked at this since @xen2 said he would make some changes. I have no idea what he intended. I did try updating this branch though I didn't check if it compiled/still "works".

@dfkeenan
Copy link
Contributor Author

I have updated the branch from master again. I actually built/tested that what I have done so far still "works".

@dfkeenan
Copy link
Contributor Author

I have made some progress but I am not sure if the changes made are desirable.

It now:

  • Marks referenced assets as referenced in the Asset View. However the asset in question does not get included in the package.
  • Renaming or moving the referenced asset does update the UrlReference. However deleting the asset does not.

I have not tested a wide selection of use cases yet. i.e. Currently only tested the UrlReference<T> class not the UrlReference one. Have not tested members of collections (List<UrlReference<Scene>>). Have not tested what happens when the referenced asset is in a package dependency.

@dfkeenan
Copy link
Contributor Author

So I decided another go in a different branch UrlReferenceAlt. I still changed a lot of things as you can see from the commit

Excluding unit tests and documentation this pretty much checks all the boxes. However there is an outstanding issue with syncing the editor view with. I have not worked out why it tries to do this conversion.

System.ArgumentException: Object of type 'Xenko.Engine.Scene' cannot be converted to type 'Xenko.Core.Serialization.UrlReference`1[Xenko.Engine.Scene]'.
   at System.RuntimeType.TryChangeType(Object value, Binder binder, CultureInfo culture, Boolean needsSpecialCast)
   at System.Reflection.MethodBase.CheckArguments(Object[] parameters, Binder binder, BindingFlags invokeAttr, CultureInfo culture, Signature sig)
   at System.Reflection.RuntimeMethodInfo.InvokeArgumentsCheck(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
   at Xenko.Core.Reflection.PropertyDescriptor.Set(Object thisObject, Object value)
   at Xenko.Core.Quantum.MemberNode.Update(Object newValue, Boolean sendNotification)
   at Xenko.Core.Quantum.MemberNode.Update(Object newValue)
   at Xenko.Editor.EditorGame.ContentLoader.LoaderReferenceManager.ReferenceAccessor.Update(Object newValue)
   at Xenko.Editor.EditorGame.ContentLoader.LoaderReferenceManager.<ReplaceContent>d__11.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
   at Xenko.Editor.EditorGame.ContentLoader.EditorContentLoader.<<CheckAssetsToReload>b__51_0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
   at Xenko.Assets.Presentation.AssetEditors.GameEditor.Services.EditorGameController`1.<>c__DisplayClass72_0.<<PostTask>b__0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
   at Xenko.Core.MicroThreading.MicroThread.<>c__DisplayClass52_0.<<Start>b__0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at Xenko.Core.MicroThreading.Scheduler.Run()
   at Xenko.Engine.Processors.ScriptSystem.Update(GameTime gameTime)
   at Xenko.Games.GameSystemCollection.Update(GameTime gameTime)
   at Xenko.Games.GameBase.Update(GameTime gameTime)
   at Xenko.Editor.EditorGame.Game.EditorServiceGame.Update(GameTime gameTime)

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.

None yet

3 participants