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

New Logging Solution #1653

Merged
merged 29 commits into from
Dec 8, 2016
Merged

New Logging Solution #1653

merged 29 commits into from
Dec 8, 2016

Conversation

koosemose
Copy link
Collaborator

@koosemose koosemose commented Dec 1, 2016

Per discussion in #1642, this replaces Uberlogger with a semi-custom logging solution (based on https://github.com/Valian/UnityDebugger ).

Similar to Uberlogger this uses channels, but rather than a custom console and still printing everything to the standard Unity Console this ONLY prints if the channel is turned on (this does have the downside that if a channel is off when an error happens, that message is gone forever). It also has support for different logging levels (so only showing warnings and errors but not standard logs), but there's not presently UI support for that.

The UI is simple and ugly, but it works (if someone more proficient in the EditorWindow UI system can make things nicer, feel free).
image

The channels are generated dynamically from logging calls being made, and stored in a ScriptableObject (Resources/ChannelSettings) so the game has to be ran at least once (and with an appropriate logging call) to fill out the channels, however, in addition to toggling all channels on/off, All acts as a default, so any new channels will use its setting.

The actual UnityDebugger code is compiled as a DLL, and hosted separately ( https://github.com/koosemose/UnityDebugger ), this causes double clicking on a log line to take you to the appropriate line, rather than the line in the logger.

Closes #1642

@BraedonWooding
Copy link
Collaborator

The UI looks good, but the icons go off the screen? Maybe make them then go down and do a second layer of buttons? Idk how feasible that is.

@koosemose
Copy link
Collaborator Author

The problem with that is that since the channels are created dynamically based off of what is used, and aren't a predictable length, I can't simple say something like 'After this channel, create a new horizontal' or 'after X number of channels, create a new horizontal', and further I have no idea how/if I can get the current size of the window and the size of the toggle labels to do any kind of reasonable calculation.

It may be possible, but my knowledge of the gui used by editor windows is far too limited to do so.

@hcorion
Copy link
Contributor

hcorion commented Dec 2, 2016

Perhaps this forum post might be helpful with getting window size: https://forum.unity3d.com/threads/editorguilayout-get-width-of-inspector-window-area.82068/

@koosemose
Copy link
Collaborator Author

koosemose commented Dec 3, 2016

Well that gets me something that is kind of close to the window size... and I can get something that I think might be the control sizes... but due to the way immediate mode gui works... it doesn't like when I try to change the layout based on things like window and control width, and not everything seems to actually be there because of the different passes that do things differently... in short, if it's possible to do what I want in the IMGui, I don't have the knowledge to do so, and don't have a good enough knowledge base to even figure out how I might do so... and since IMgui is now only used for editor stuff, resources are quite thin... I've spent 5 hours (this time around) and the best I can get is an approximate, rough estimate of the reason why I can't do what I want... so until someone who actually understands IMGui comes along to fix it, we're gonna have an ugly control and we can just make the window bigger if things don't fit.

Best idea I have is seeing if I can put a scrollbar in somewhere.

@bjubes
Copy link
Collaborator

bjubes commented Dec 3, 2016

If it helps, the old GUI system was built using the same code that builds the current editor GUI, so maybe look up some old tutorials about unitys GUI system, before 5.0?

@bjubes
Copy link
Collaborator

bjubes commented Dec 3, 2016

Also when I check "all", i still don't get any log messages...

this may be because I have two unitys installed side by side

@koosemose
Copy link
Collaborator Author

I've tried looking for older stuff, but to no avail... I've found it, but none of it's really covered what I need. I could in theory go through learning every in and out of the system and do it, but to be honest that's entirely too much work for something that may not be possible with the system and for something I am unlikely to need to use much (if any) in the future.

@zwrawr
Copy link
Contributor

zwrawr commented Dec 3, 2016

Is adding a horozonal scroll bar possible if it is, it might be easier?

@bjubes
Copy link
Collaborator

bjubes commented Dec 3, 2016

Is there any way to keep uberlogger but just skip passing messages to Unity's Log?

@bjubes
Copy link
Collaborator

bjubes commented Dec 3, 2016

your gonna hate this, but deleting these lines in UberLogger.cs:449 :

// If required, pump this message back into Unity
if (ForwardMessages)
{
    ForwardToUnity(source, severity, message, par);
}

keeps UberLogger working fine and unity doesn't log anything. And If we want Unity to log only errors and warnings, we can easily implement that instead:

if (severity != LogSeverity.Message)
{
    ForwardToUnity(source, severity, message, par);
}

fps remain fairly stable when doing this and spamming ULogChannel calls in update. If you're not on the channel, its hardly noticeable performance wise.

@koosemose
Copy link
Collaborator Author

That still leaves us with the rest of Uberlogger's issues. Most notably its common failure to give a full callstack on error, of course said error is still in the regular console, however then we basically end up with on console solely for errors, and a separate one for errors. The issues with multiline logging. The inability to expand the callstack subwindow.

Personally I would prefer to continue on with Uberlogger duplicating the log messages to than to have it not print to the console and have to deal with its terrible UI. At least with my terrible UI you only have to use it when toggling channels.

@bjubes
Copy link
Collaborator

bjubes commented Dec 3, 2016

I misunderstood your reasoning for this PR. I was focused on this

[Uberlogger] causes a lot of slow downs that gives a false impression of the game's performance.

from your issue but I didn't realize all the other advantages your filter has. Since Uberlogger isn't slow using what I did above, should we keep it alongside this PR for its good features (searching, jumping to each line in the call stack, etc...) or just scrap it entirely?

@koosemose
Copy link
Collaborator Author

They could in theory live side by side. It would of course require some tweaks to my code (likely something along the lines of a bool to determine where to dispatch the log maybe?)

Regarding the issue you mentioned earlier, I've tried to emulate it, but can't get an issue with it. Just to see if things are behaving as they should, "All" should do two things, serve as the default for any new channels (any which aren't currently in the display), and toggle on/off any existing channels. It won't override an individual channels settings (so you you check all, then uncheck things, those channels won't show even though "All" is checked.

@zwrawr
Copy link
Contributor

zwrawr commented Dec 3, 2016

If you unchecked a channel when all is check, it should unchecked all. (and if everything is checked then all should get checked)

@koosemose
Copy link
Collaborator Author

I disagree, all controls the default state of new channels, having it be toggled back off because you turned off a channel would lead to unwanted results.

@bjubes
Copy link
Collaborator

bjubes commented Dec 3, 2016

Is there any way to make it optionally stack vertically? that might make the size of the window more manageable and predictable since each added channel only will take one line of text.

Edit: I just tried it myself. Heres how it looks.
capture
Personally I like it more, but a toggle between vertical and horizontal is probably best. I sent you a PR of what I did to make it vertical.

@BraedonWooding
Copy link
Collaborator

I really like what bjubes did btw. Also I think that the "All" button should toggle on/off all the individual components? Or you could just say If 'All' or [The other relevant channel] then display message?? It's a little unintuitive for it just to control the default state?

@koosemose
Copy link
Collaborator Author

koosemose commented Dec 3, 2016

Hmm yeah, that does look pretty good... yeah, making it vertical is easy (it's the default). I originally made it horizontal, because I mistakenly assumed it would wrap around, and from there had it set in my head as being that way so I didn't consider making it vertical. I'm not sure if it's feasible to have it changeable between horizontal and vertical, as things that change the layout on the fly tend to be a little unreliable.

@BraedonWooding It does toggle all channels on or off in addition to serving as the default.

@bjubes
Copy link
Collaborator

bjubes commented Dec 4, 2016

@BraedonWooding I agree with @koosemose. When explained, the way the all option works sound counterintuitive, but when you actually test for a while with it, the way its implemented makes the most sense. The best way to describe the "all" behavior is how select all works in programs like blender and Photoshop. If some but not all is selected, clicking it selects everything, and clicking again deselects everything.

If there was a way for "all" to change to a semi checked box, that would make more sense visually, but that's not possible in unity.

@BraedonWooding
Copy link
Collaborator

I just tested it out and I do have to say I agree with you know it does make more sense (Unity really should have a semi-checkbox).

@bjubes
Copy link
Collaborator

bjubes commented Dec 5, 2016

Im going to add the feedback label to get any last minute suggestions before this gets merged. I think this is good to go, but i want to give it some time to make sure everyone knows whats going on when uberlogger suddenly disappears in master

@BraedonWooding
Copy link
Collaborator

Tip update the image? Just so anyone who comes by can see the current UI?

@bjubes
Copy link
Collaborator

bjubes commented Dec 8, 2016

I think this is ready to be merged, its been up for a week now, but there are conflicts.

@bjubes bjubes changed the title New Logging Solution [Conflicts] New Logging Solution Dec 8, 2016
@koosemose koosemose changed the title [Conflicts] New Logging Solution New Logging Solution Dec 8, 2016
@koosemose
Copy link
Collaborator Author

Conflict Resolved

@bjubes bjubes merged commit d051088 into TeamPorcupine:master Dec 8, 2016
@koosemose koosemose deleted the Use-UnityDebugger branch December 9, 2016 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants