search

vrchat-community / UdonSharp

A compiler for compiling C# to Udon assembly
https://udonsharp.docs.vrchat.com
MIT License
470 stars 50 forks source link

Fixes errors pointing at LogError instead of project asset #86

Open Faxmashine opened 1 year ago

Faxmashine commented 1 year ago

@MerlinVR should weigh in before I proceed further.

This fix makes it easier to find U# program assets without a source C# asset. Clicking the error takes the user to the asset, instead of UdonSharpUtils.cs's LogError.

MerlinVR commented 1 year ago

🤔 the program asset was already passed as context to that error? Changing the text to be more specific is fine, but introducing an overload that takes U# program assets for the UdonSharpUtils.LogError() is redundant since one already exists for generic UnityEngine.Objects. It's just not great when you need to point to an asset with the context since usually people will have the Console window and Project window on different tabs in the same window so clicking the error only brings up the inspector for the asset and doesn't select it in the Project window. Also fyi AssetDatabase.FindAssets() gets pretty slow to call on large projects so it's usually avoided or cached. It also tends to... not work.

Faxmashine commented 1 year ago

Oh yeah, we should avoid needlessly finding assets twice, and the overload sucks.

If we don't want to add the proper context we should discard the context, as seeing the logger is useless (or misleading) for creators.

I think the content may get lost because there's some array cloning going on when all the assets are first retrieved.

On Mon, Dec 5, 2022, 17:37 Merlin @.***> wrote:

🤔 the program asset was already passed as context to that error? Changing the text to be more specific is fine, but introducing an overload that takes U# program assets for the UdonSharpUtils.LogError() is redundant since one already exists for generic UnityEngine.Objects. It's just not great when you need to point to an asset with the context since usually people will have the Console window and Project window on different tabs in the same window so clicking the error only brings up the inspector for the asset and doesn't select it in the Project window. Also fyi AssetDatabase.FindAssets() gets pretty slow to call on large projects so it's usually avoided or cached. It also tends to... not work.

— Reply to this email directly, view it on GitHub https://github.com/vrchat-community/UdonSharp/pull/86#issuecomment-1337688339, or unsubscribe https://github.com/notifications/unsubscribe-auth/A36FF4HYNO4CV3ZOQCRVRYDWLYK5NANCNFSM6AAAAAASUM5NJ4 . You are receiving this because you authored the thread.Message ID: @.***>

MerlinVR commented 1 year ago

It does point to the asset properly when I've tried it, is there some repro for it not working?

Faxmashine commented 1 year ago

Repro steps

  1. Create an UdonSharp project with the VCC
  2. Create an UdonSharp script (Project window -> right click -> U# script)
  3. Delete the C# script
  4. Double click either of the two errors

UdonSharpUtils opens when it shouldn't

MerlinVR commented 1 year ago

I can get it to repro sometimes with that, it looks more like Unity's Context only works if there aren't multiple errors pointing to the same thing in a row or something along those lines. Loading the asset doesn't improve it either so I'm assuming your fix just seemed to change because the behavior of Unity is inconsistent. I think this is just a Unity moment and we can't do much about it.

MerlinVR commented 1 year ago

https://user-images.githubusercontent.com/36685500/205764202-3a19c90d-8546-4417-9110-df7e534ed485.mp4

To illustrate