[ButtonMethod] not work in list item

[CharlieDreemur]

[ButtonMethod] do works in an independent item,
but it fail when come to the list item in a list/dictionary

[jinda-li]

It happened to me recently. In a button method I do something like list.Add() and list.Clear(), but the list is messed up:

1. The list in the prefab is changed, even though I did not change the prefab.
2. Non-null items ref in the list are cleared when clicking Play. Null items are still in the list.
   Hope it can be fixed soon

[jinda-li]

I think it might be related to prefabs. I found that, if the game object is not from a prefab, this button method in editor works fine, the value is not lost after playing.
[ButtonMethod] void FindCollider() { m_Collider = GetComponent<Collider>(); }
However, if the gameobject is from a prefab. In editor, the change made by this method is not registered. It will be lost after playing.

I guess RecordPrefabInstancePropertyModifications() or SetDirty() would help. I will try it later when I have time.

[Deadcows]

[ButtonMethod] do works in an independent item, but it fail when come to the list item in a list/dictionary

Unfortunately, ButtonMethod is restricted to the root of MonoBehaviour or ScriptableObject. It wouldn't work with serialized custom type, but it will be shown through DisplayInspector Attribute. So, if you will use SO or MB in a list, you can do it like that (elements of the list are child MBs):

I know it's not a universal solution, but it is the only way, currently, to display buttons in a list.
Nowadays I'm using Odin inspector to do this kind of things, it works fine along with MyBox.
Extending this attribute to work with any serialized type is possible, but not in my priority. Would appreciate any contributions to the codebase 😅🙌🏻

---

I think it might be related to prefabs. I found that, if the game object is not from a prefab, this button method in editor works fine, the value is not lost after playing. [ButtonMethod] void FindCollider() { m_Collider = GetComponent<Collider>(); } However, if the gameobject is from a prefab. In editor, the change made by this method is not registered. It will be lost after playing.

Yep, you should definitely mark dirty any object, if you change its serialized data in the code during edit-time. Unity doesn't know that you changed the m_Collider field or resized the serialized list, so it won't save changes on the disc and they will be wiped out on domain reload.
The reason why changes weren't lost the first time is probably because you changed something else via inspector (this way the object was marked dirty internally) and all serialized data was successfully saved 🤔.
Something like this as a rule of thumb:

#if UNITY_EDITOR
[ButtonMethod] void FindCollider() { 
    m_Collider = GetComponent<Collider>();
    UnityEditor.EditorUtility.SetDirty(gameObject);
 }
#endif

[jinda-li]

In this page: https://docs.unity3d.com/ScriptReference/EditorUtility.SetDirty.html

If the object may be part of a Prefab instance, you additionally need to call PrefabUtility.RecordPrefabInstancePropertyModifications to ensure a Prefab override is created.

A prefab instance works with this piece:

#if UNITY_EDITOR
[ButtonMethod] void FindCollider() { 
    Undo.RecordObject(this, "Assign m_Collider");
    m_Collider = GetComponent<Collider>();
    PrefabUtility.RecordPrefabInstancePropertyModifications(this);
 }
#endif

Undo.RecordObject() would be better than UnityEditor.EditorUtility.SetDirty()
Otherwise I need to use SerializedProperty system, but I am not really into that more complex way. I recommend you add some more notes to https://github.com/Deadcows/MyBox/wiki/Attributes#buttonmethod so that others know it as well. 😄

[jinda-li]

@Deadcows Also, would you like if add these two lines to ButtonMethodAttribute.cs?

private void InvokeMethod(Object target, MethodInfo method)
{
	Undo.RecordObject(target, method.Name);   //New Line
	var result = method.Invoke(target, null);
	PrefabUtility.RecordPrefabInstancePropertyModifications(target);  ////New Line
	if (result != null)
	{
		var message = $"{result} \nResult of Method '{method.Name}' invocation on object {target.name}";
		Debug.Log(message, target);
	}
}

I am not pretty sure this would fit every cases of this issue😂, at least I tried myself it works with scene objects/ prefab instance/ modification of other objects in the scene.

[Deadcows]

I don't like the idea that it always makes the scene dirty. I tried that and it made some undesired behaviors in my previous project 🤔 (several game designers pushed a bunch of changed scenes in vcs causing a lot more conflicts that it should 😅)

Maybe it's better to dirty it if the "result" is not null and cast to "true"? So, if ButtonMethod returns bool and explicitly indicates that the scene should be marked dirty as a result of its invocation 🤔

So, usage will be like this:

[ButtonMethod]
private bool FixTheName() {
    var newName = GetValidName();
    if (newName == name) return false;
    name = newName;
    return true;
}

Would be optional, and easily explained in docs. Nicely combined with ContentsMatch api...
What do you think?

[jinda-li]

However the return type of this method is restricted to bool 🤔. If a method written before returns bool, after this update may cause unexpected results.
Would an optional parameter of this attribute help? For example:

//ButtonMethodAttribute(ButtonMethodDrawOrder drawOrder = ButtonMethodDrawOrder.AfterInspector, bool recordModifications = false)
[ButtonMethod( ButtonMethodDrawOrder.AfterInspector, true)]
private void Method(){
    //...
}

If recordModifications is explicitly set to true, then these two methods can be called before and after invoking this button method:

...
Undo.RecordObject(target, method.Name);   //New Line before invoking
PrefabUtility.RecordPrefabInstancePropertyModifications(target);  //After invoking, necessary for prefabs
...

Nice discussion! Also, I like your games! 😊

[Deadcows]

Yep, backward compatibility is a thing I always forget about 😅 I don't like the optional parameter thing, because the decision to make dirty often comes in the body of the method. Why not create a custom type as a flag for a ButtonMethod, then?

This will allow to keep the default behavior with logging for all other types 👍🏻
like here (not sure about the naming though):

public enum MakeDirty { True, False } //Part of MyBox
...
[ButtonMethod]
private MakeDirty Method(){
    //...
    return MakeDirty.True;
}

btw, is it safe to call RecordPrefabInstancePropertyModifications on any objects or we should check if target object is part of a prefab first? And it also probably should only be called during prefab-editing-mode, to not force the changes made on prefab instance 🤔

---

Nice discussion! Also, I like your games! 😊

Thank you, mate! Working hard on the third project, so not have that many time to spend on MyBox now. But I'll make a big update when sort all the cool things I made during the work on The Bookwalker 🙂

[jinda-li]

@Deadcows
I realized that Undo.RecordObject(target, method.Name) is called before invoking the method, so probably the return value of the method can not decide if it should record undo or set dirty (set dirty does not record undo).

private void InvokeMethod(Object target, MethodInfo method)
{
	Undo.RecordObject(target, method.Name);
	var result = method.Invoke(target, null);
	PrefabUtility.RecordPrefabInstancePropertyModifications(target);
	if (result != null)
	{
		var message = $"{result} \nResult of Method '{method.Name}' invocation on object {target.name}";
		Debug.Log(message, target);
	}
}

From my own experience, most of time a button method does something about changing serialized values (from Monobehavior, ScriptableObject or a serializable class). I would say by default RecordObject() and RecordPrefabInstancePropertyModifications() should be called to support most cases. And also to keep it neat, I would recommend this way:

//In mybox
//ButtonMethodAttribute(ButtonMethodDrawOrder drawOrder = ButtonMethodDrawOrder.AfterInspector, bool recordModifications = true)  

...
//Be default, record modifications.
[ButtonMethod]
private void Method(){
    //...
}

//If a developer really does not want it to be recorded, note that every change will be lost after playing.
[ButtonMethod( ButtonMethodDrawOrder.AfterInspector, false)]
private void MethodNoModification(){
    //...
}

btw, is it safe to call RecordPrefabInstancePropertyModifications on any objects or we should check if target object is part of a prefab first? And it also probably should only be called during prefab-editing-mode, to not force the changes made on prefab instance 🤔

I tried in my project, RecordPrefabInstancePropertyModifications() is safe to call for a gameobject in scene. I think Unity does some checking internally.

• If it is a prefab instance in scene, the change is made to the instance itself, which is necessary, otherwise lost after playing. The prefab is not changed.

  • Prefab instance in scene:
    
  • Prefab Prefab-edit-mode:
    

• If it is prefab-editing-mode, the change is record to the prefab.

What would you think?❤️