Skip to content

Conversation

RyotaMurohoshi
Copy link
Contributor

Change PrefabBrushes Erase behaviour with small doc change.

Overview

In this PR, I

  1. Changed PrefabBrush and PrefabRandomBrush Erase behaviour. (and changed Paint implementation)
  2. Added PrefabRandomBrush in README.md
  3. Fixed MenuItem menuName and fileName for PrefabBrush and PrefabRandomBrush.

Background

From name of PrefabBrush, I expect that PrefabBrush can only erase GameObject that is instantiated from the prefab.

By the way, current PrefabBrush can delete

  • If ForceDelete is true, PrefabBrush can erase any GameObject in the cell.
  • If ForceDelete is false, PrefabBrush can erase any GameObject that have same name for the prefab.

This behaviours is not related to "Prefab". So I suggested this PR.

Detail for changing Erase behaviour.

PrefabBrush's Erase

Current:

  • If ForceDelete is true, PrefabBrush erases any GameObject in the cell.
  • If ForceDelete is false, PrefabBrush erases any GameObject that have same name for the prefab.

My Suggestion:

PrefabBrush erases GameObject that is instantiated from the prefab.
To check GameObject that is instantiated from the prefab or not, PrefabBrush uses PrefabUtility.GetCorrespondingObjectFromSource
And delete ForceDelete field.

PrefabRandomBrush's Erase

Current:

PrefabBrush erases any GameObject in the cell.

My Suggestion:

PrefabBrush erases GameObject that is instantiated from the any prefab in prefab lists.
To check GameObject that is instantiated from the prefab or not, PrefabBrush uses PrefabUtility.GetCorrespondingObjectFromSource

Conclusion

From name of PrefabBrush, I expect that PrefabBrush can only erase GameObject that is instantiated from the prefab.

@RyotaMurohoshi
Copy link
Contributor Author

Additional memo,

Deleting "Prefab protected field" is breaking change for API.
But this change is not included version up.
So I want to delete it.

@ChuanXin-Unity
Copy link
Collaborator

Great suggestions!

Using PrefabUtility.GetCorrespondingObjectFromSource to identify if the Prefab to be erased and the instance in the Scene is a better way to check if they are the same.

However, I think keeping the ForceDelete functionality would be useful. This would allow users to quickly remove a different Prefab and replace it with one/s from the Brush without switching to a different Brush or removing it using the Hierarchy window. Finding the right Brush to erase from and switching back may take a while.

Let me know what you think about this! Thanks!

@RyotaMurohoshi
Copy link
Contributor Author

RyotaMurohoshi commented May 9, 2020

@ChuanXin-Unity Thank you!

However, I think keeping the ForceDelete functionality would be useful.

Yeah, I agree that the feature to erase GameObject on Grid or Tilemap is useful 👍

This would allow users to quickly remove a different Prefab and replace it with one/s from the Brush without switching to a different Brush or removing it using the Hierarchy window.
Finding the right Brush to erase from and switching back may take a while.

GameObjectBrush has the feature to erase GameObject on Grid or Tilemap.
It is so nice 👍
And I can expect this behaviour from name of GameObjectBrush.

By the way, PrefabBrush's erase feature is a little difficult to expect.
With name of PrefabBrash, I can not expect that ForceDelete feature behaviour until I read the code.

In my thought, PrefabBrash should has only feature to handle Prefabs (from the name of PrefabBrush.)
And If we want to erase GameObjects on Grid or Tilemap with brushes, how about using GameObjectBrush.


edited

By the way, If you think that PrefabBrush needs the feature to erase GameObject force, I will add the feature in this PR.
Please let me know 😄

@ChuanXin-Unity
Copy link
Collaborator

By the way, If you think that PrefabBrush needs the feature to erase GameObject force, I will add the feature in this PR.

Yes, please! If possible, we should try to change the behaviour and name of ForceDelete to something like DeleteOwnedPrefabOnly.

@RyotaMurohoshi
Copy link
Contributor Author

RyotaMurohoshi commented May 16, 2020

@ChuanXin-Unity

Hi! I added the feature to erase any GameObjects to Prefab Brush.

By the way, I changed implementation from before one. So let's talk about spec.

Previous Implementation.

forceDelete bool value is stored in PrefabBrush Assest.
If forceDelete is true, PrefabBrush erases any GameObject in the cell.
If forceDelete is false, PrefabBrush erases any GameObject that have same name for the prefab.

My Suggestion Implementation

Any boolean flag is not stored in PrefabBrush Assest.
If "Erase Any Objects" checkbox in TilePalette Window is true, erases any GameObjects that are in a given position within the selected layers.
If "Erase Any Objects" checkbox in TilePalette Window is false, erases only GameObjects that are created from owned Prefab in a given position within the selected layers.

I want to avoid that "forceDelete" boolean value storing in PrefabBrush Asset.
In my expect, "forceDelete" flag will be changed per usage timing or per user.
So I want to avoid storing boolean "forceDelete" value.
In next situation, storing "forceDelete" boolean value in PrefabBrush Asset is not good.

  1. There are 2 team members who use a PrefabBrush asset.
  2. User A changes "forceDelete" flag to true and commit & push to repo.
  3. User B changes "forceDelete" flag to false and commit & push to repo.
  4. Unexpected conflict will be happen.

"DeleteOwnedPrefabOnly" is good name. But I want to set CheckBox name "Erase Any Objects".

The reason why I chose "Erase Any Objects" are

  • I want to set default behaviour as erasing only GameObjects that are created from owned Prefab.
  • I want to set default value as false.
  • I choosed name Erase not Delete from Tile Painting feature name.

@ChuanXin-Unity
Copy link
Collaborator

This is great! Erase Any Objects sounds clearer as well!

It is also good that you mentioned user preferences and serialized properties when used with source control repositories. I will keep that in mind too!

Thank you for your help!

@ChuanXin-Unity ChuanXin-Unity merged commit ab1eea6 into Unity-Technologies:master May 20, 2020
@RyotaMurohoshi RyotaMurohoshi deleted the update_prefab_brushes branch May 20, 2020 10:17
@RyotaMurohoshi
Copy link
Contributor Author

🎉

Thanks!

@vladderb
Copy link
Contributor

@RyotaMurohoshi

Thank you for adding this functionality and fixing my mistakes! For me it was the first pull request and it's nice to see that people from different countries can jointly develop open source software solutions.

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.

3 participants