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

[GameStudio] Improve "Add component" button usability. #411

Merged
merged 22 commits into from Apr 13, 2019

Conversation

@dfkeenan
Copy link
Contributor

dfkeenan commented Mar 16, 2019

PR Details

This change is to replace the current "Add component" ComboBox with a searchable list.

Description

This PR is to change the "Add component" button searchable, similar to how "Add asset" works.

Add Component

Related Issue

None

Motivation and Context

The current ComboBox can make it hard to find the components you are looking for, especially if your project contains a lot of scripts.

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

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Mar 16, 2019

Things to think about:

@dfkeenan

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Mar 16, 2019

I created a new ComponentCategoryAttribute and marked "all" the components. I put the EntityComponent in the "Miscellaneous" category so all components will default to this category. Where possible I only put the base classes in a category. i.e. I added "Scripts" to ScriptComponent so StartupScript, SyncScript, AsyncScript and all custom user scripts will be in this category. (Users can override the category for there scripts if they desire.)

  • Audio
    • AudioListenerComponent
    • AudioEmitterComponent
  • Lights
    • LightShaftBoundingVolumeComponent
    • LightProbeComponent
    • LightShaftComponent
    • LightComponent
  • Miscellaneous
    • EntityComponent
  • Scripts
    • ScriptComponent
  • Animation
    • AnimationComponent
  • Model
    • ModelNodeLinkComponent
    • ModelComponent
  • Navigation
    • NavigationBoundingBoxComponent
    • NavigationComponent
  • Particles
    • ParticleSystemComponent
  • Physics
    • PhysicsComponent
  • Sprites
    • SpriteComponent
    • SpriteStudioNodeLinkComponent
    • SpriteStudioComponent
  • UI
    • UIElementLinkComponent
    • UIComponent
  • Video
    • VideoComponent
@dfkeenan dfkeenan force-pushed the dfkeenan:Add-Component-vNext branch from 8468d5e to c96b658 Mar 16, 2019
@dfkeenan

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Mar 16, 2019

I have run the existing unit tests that I could for the projects which I changed. I couldn't run Xenko.Core.Presentation.Tests because it's missing the test adapter package. (I fixed this in #400). There didn't seem to be any tests for Xenko.Assets.Presentation. I have not created new unit tests for these changes.

For now I am going to mark this PR as ready for review and start updates to the manual.

@dfkeenan dfkeenan marked this pull request as ready for review Mar 16, 2019
@profan

This comment has been minimized.

Copy link
Contributor

profan commented Mar 16, 2019

Can we make it so we can create new scripts and add them to the entity from the "Add component" dropdown?

(already sort of discussed, and would otherwise need two rounds of docs updates if done in a separate PR from this, to save our poor dfkeenan from so much docs work)

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Mar 16, 2019

@profan you mean, it would open the create script dialog, and then use this one?
Wondering how well that would work, because it would need to compile+reload assembly as an intermediate step for this to work, and doing so can have some side effects (usually we ask user before doing so).

@profan

This comment has been minimized.

Copy link
Contributor

profan commented Mar 16, 2019

@xen2 Right, that would be the idea.

For me at least it's relatively common to create a script only for a specific entity and add it directly to that object, or just when you start working it's always meant for one entity either way.

It might be messy technically but it'd be a good workflow win I'd say, @dfkeenan can also chime in with their thoughts here I'm sure 👀

@dfkeenan

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Mar 16, 2019

The ScriptTemplateGenerator sort of handles some of that stuff. I think it at least kicks off the "do you want to save/reload" process. But I hadn't gotten as far as working out how to make the command wait until the project has been reloaded before adding the component to the entity.

@xen2 , If you are happy with the general design idea I can investigate/experiment. I just don't want to add features you don't want.

If it is feasible we have to decide on where to button goes. i.e Is it just always there beside the search text box or appear on the right hand side where the components list displays only if all components have been filtered out over .

@phr00t

This comment has been minimized.

Copy link
Contributor

phr00t commented Mar 17, 2019

I pulled this into my branch & tried it out. Really cool, I like it!

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Mar 17, 2019

@dfkeenan @profan Sure, I agree the workflow would be nice. Feel free to give it a try!

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Mar 17, 2019

FYI, I will take the time to review this properly right after GDC. Sorry for the delay.

@dfkeenan

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Mar 17, 2019

That's OK. I will try having a go at adding the ability to create scripts directly from Add component popup this coming weekend.

Also lots of images to update in documentation. 👀

@dfkeenan

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Mar 23, 2019

Started work on adding the ability to create scripts. It currently doesn't add the new script to the entity because I haven't worked out how to wait for the assemblies to be reloaded. Also doesn't let you choose where the script will be created.

CreateScript

@dfkeenan

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Mar 30, 2019

I have managed to get it so it now creates the script component and add it to the entity.

I made a change to SessionViewModel and DebugingViewModel to do this. I am not really happy about what I have done here. (I wouldn't be surprised if didn't pass review.) I couldn't really think of a better solution, though one may have been to come up with an interface and register DebugingViewModel as a service. The reason I made these changes is because the new script cannot be added as a component until the project has been saved, built and the assemblies reloaded.

As it stands it does create and add the new script component once user saves/reloads assemblies and Undo/Redo seems to work as expected.

My biggest concern (apart from the above mentioned changes to SessionViewModel etc.) is how well things work if the user says No to the save and/or reload prompts. I have tried various things and have not managed to break it yet. i.e.:

  • Create new folder and move source file, then save/reload.
  • Add a different component to same entity i.e. a model, then save/reload.
  • Create/add another new script, then save/reload.

Some things that do not work if you do before save/reload is things like moving entity in the scene hierarchy, Cut & Past entity.

Probably the only way to make this rock-solid with the way GS currently reference script entities would be to change it so it doesn't give you a chance to opt out of save and reload. (Prompt before new script window is shown.) Would we really want to go down that path?

@dfkeenan

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Apr 6, 2019

So I have reverted changes to DebbingViewModel and SessionViewModel as well as commenting out the button for and registering of the AddNewScriptComponentCommand. As getting this to work properly requires some big changes.

I was going to mark this as ready for review. But apparently it's not a draft PR anymore?? 🤔

Is it worth updating the MANY MANY images in the documentation now if I intend to create a new PR for adding the create script bit? Probably depends on how soon you intend to make 3.1 RTM and whether or not I can do the next part before then. (I do have a PR for the start I made on the documentation covering the new attribute usages etc.)

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Apr 7, 2019

Did a few tests, it works pretty well and looks almost ready for merge, thanks!

I have noticed 2 small windowing behavior issues:

  • If it's open and then doing alt-tab, it stays as topmost windows (on top of the new window)
    "Add asset" doesn't exhibit this behavior
  • If I select another dock pane (such as Asset View), and I click "Add component", it doesn't open (only after the second click)
@dfkeenan

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Apr 7, 2019

Ok, I will have a bit of a look and see if I can work out what's going wrong. I did notice some windowing issues but I thought that was being caused by me debugging it.

@dfkeenan

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Apr 7, 2019

I had a closer look at "Add asset" and changed things to be more like that and look to have solved those problems. I also tidied up the xaml format while I was at it.

# Conflicts:
#	sources/editor/Xenko.Assets.Presentation/Xenko.Assets.Presentation.csproj
@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Apr 8, 2019

I still notice two other small weird behaviors:

  • If I click Add Component twice (open and close) then click anywhere else, it keeps reopening it and closing it one more time.
  • If I click on a category name, it closes the Add Component popup.

PS: When you merge will latest master, you will need to undo all changes to Xenko.Assets.Presentation.csproj (xaml files are now automatically included since #406)

@dfkeenan

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Apr 8, 2019

More bugs hey. I guess I'll try to fix them after I workout how to update my branch.

I really need to learn how to use git properly.

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Apr 9, 2019

@dfkeenan I pushed the merge commit with latest master in your branch, so you should be up to date by simply updating to latest version of your branch.

@dfkeenan

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Apr 9, 2019

@xen2 , thanks for the help but now I'm a bit confused. So now I just have to do a pull from my forks branch into my local branch?

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Apr 9, 2019

Yes, a git pull with fast-forward (should be default) is fine
Just like if you follow any other branch that has been updated remotely.

@dfkeenan

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Apr 9, 2019

OK, I'll try that tonight after work and see how I go.

@dfkeenan

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Apr 9, 2019

@xen2 , I have fixed the second issue about clicking on category names. Not sure about the other one add asset does the same thing. I have also introduced a new "bug" that add asset also has. The escape key doesn't close the popup anymore.

@dfkeenan

This comment has been minimized.

Copy link
Contributor Author

dfkeenan commented Apr 13, 2019

So the weird behavior when you click the toggle button twice seems to be caused by the ToggleButtonPopupBehavior which I added because the "Add asset" does the same. If I remove this it works like how I would expect. I will commit/push this as well. @xen2 , you might want to merge these last to commits.

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Apr 13, 2019

Thanks. Turns out "Add Asset" also has the bug with "If I click Add Asset twice (open and close) then click anywhere else, it keeps reopening it and closing it one more time."
Thinking about it, scene editor toolbar popup also behaves weirdly sometimes.

Noticed a few last things:

  • Now, Clicking "Add Component" twice closes and reopens it again. That doesn't feel natural (instead of either closing it like before, or otherwise at least keeping it open?)
    Maybe it might be worth readding and taking a look at ToggleButtonPopupBehavior (it would also fix Add Asset issues mentioned previously)
  • Category choice and filtered right pane is kept when reopening (instead of always opening with no right pane like Add Asset does)
@xen2 xen2 merged commit a569a20 into xenko3d:master Apr 13, 2019
2 checks passed
2 checks passed
WIP ready for review
Details
license/cla Contributor License Agreement is signed.
Details
@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Apr 13, 2019

Merged, thanks!

@dfkeenan dfkeenan deleted the dfkeenan:Add-Component-vNext branch Apr 13, 2019
@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Apr 19, 2019

@dfkeenan noticed the style of Project Selection changed little bit: the selected text is now black (before it was white).
image
Could it be due to your Xaml changes?

@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Apr 19, 2019

Never mind, it's OK now, might have had something wrong with my previous build...

DwiYI added a commit to DwiYI/xenko that referenced this pull request May 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.