search

Qriva / MonoBehaviourTree

Simple event driven Behaviour tree for Unity projects
MIT License
238 stars 17 forks source link

Add ability to open node script in IDE from node menu #11

Closed blue-train closed 3 weeks ago

blue-train commented 8 months ago

When you open the node menu using RMB, you can choose "Edit Script" to immediately open it in your code editor.

Qriva commented 8 months ago

Hello, you propose to add "edit script" in context menu, however I have several questions and notes to this addition:

  1. Selected node has script field in the inspector that has probably the same behaviour (unless there is custom inspector). In that case is that really important to add this shortcut in the context menu?
  2. I don't think this should be the first element in the context menu, maybe second one.
  3. There is version of FindObjectsOfTypeAll that takes Type as argument, but more importantly are you sure this function cannot fail in some cases if asset is not loaded or something similar?
  4. Most importantly, should be allowed to edit scripts like this? Nodes like "Selector" are part of the package, so it would mean we explicitly encourage the user to edit scripts that should not be touched.
blue-train commented 8 months ago

Hello!

  1. The idea was taken from a Behaviour Designer, where there is the same option. When we started using MonoBehaviourTree in our project (after using Behaviour Designer on the previous one), one of the first changes was adding this ability, because there is a quicker way to open node script in IDE, despite that it can also be opened from Inspector.
  2. OK, as you decide, let it be the second one.
  3. I would like to know the benefits of using FindObjectsOfTypeAll that takes Type as an argument. According to documentation, this version will return Object[] instead of T[], and you will need to cast it on the next step, moreover, this version will be slower than generic one. Even in Editor mode (i.e. non-Playmode), it loads 2000+ scripts when I run it in the forked project. This function must return at least 1 script because at least you have compiled the source script itself, so the list will never be empty. Next, FirstOrDefault will return null in the worst case, and in this case, AssetDatabase.OpenAsset() will do just nothing.
  4. In our project, we made some changes to internal node scripts like IsSetCondition, and also we often needed to look at how other internal scripts like different Decorator implementations work, so this option was useful for us. Maybe we can change the name of the function to "Open Script" instead of "Edit Script". Moreover, the user of MonoBehaviourTree can just find the script in another way and make changes.

But if you think that this option may break the way how developer should work with your asset, then I understand.

Qriva commented 8 months ago

Hi again

  1. Ok I get you. Just curious, I guess behaviour designer is one of the best, if not best behaviour tree asset for unity, so why would you try another one, like this one?
  2. ✔️
  3. Fair enough. I haven't tested this, and there are so many methods in unity that it's possible I mixed things up. I just remember I had some problems with unloaded assets, but I guess as you said - in theory scripts should be always loaded by definition.
  4. I get from where you are coming from and this use case is totaly valid, but I wonder if this can be done better. I mean, this asset is probably used by inexperienced/hobby users, and such a menu suggests the script should or can be edited. I thought what if edit menu was visible only for scripts that are placed in project (assets folder) or by namespace. The problem is in this case you can't preview default nodes and thats not the goal. I wonder if there is way to open those files in readonly mode, I think unity does something similar in some cases, or I mixed some things and it's only for package cache files? If there is no way to do that (or at least you don't know one), then perhaps it would be good to show different menu item, "edit script" for user scripts and "open/preview script" for built in nodes. What do you think?

By the way, you mentioned you modified IsSetCondition, can you tell me what exactly? If you think this is good general improvement, maybe it should be included by default? Same for other systems and nodes if you have some fairly simple ideas to implement, I can add them.