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

Publisher: Show instances in report page #4915

Merged
merged 56 commits into from
May 23, 2023

Conversation

iLLiCiTiT
Copy link
Member

@iLLiCiTiT iLLiCiTiT commented Apr 28, 2023

Changelog Description

Show publish instances in report page. Also added basic log view with logs grouped by instance. Validation error detail now have 2 colums, one with erro details second with logs. Crashed state shows fast access to report action buttons. Success will show only logs. Publish frame is shrunked automatically on publish stop.

Additional info

Publishing report page is not very helpful and based on conversations we're trying to Paragraphs of text giving context of additional technical information or code examples. Added few smaller changes related to PySide6.

Todo list:

  • Add icons next to instances with color of their state (in progress, warning, success, error)
  • Publish actions widget did not change yet so the sizes and layout is not ideal.

Optional todo list:

  • Show/Hide logs option -> log may cause that UI is slower so it would be nice to have option to hide them.
  • Filter logs by plugin where validation error happens.
  • Add option to swap between instance and validation error views when validations are showed.
  • Logs don't have option to change filtering.

Testing notes:

  1. Open Publisher in a DCC or in TrayPublisher
  2. Try play with report page

@ynbot ynbot added size/XL Denotes a PR changes 1500-2499 lines, ignoring general files type: feature Larger, user affecting changes and completely new things labels Apr 28, 2023
@BigRoy
Copy link
Collaborator

BigRoy commented Apr 28, 2023

Great work!

Love the fact that it also doesn't overlay the higher "panel" over the report by default but it hides downwards. Nice!

Here is a current screenshots:

afbeelding


Bug: It shows logs right hand side even for plugin entries not selected left-hand side.

As you mentioned in your notes

For example here it shows logs right hand side for Validate Expected Frames Exist even though Validate Background Depth 32 Bit plugin report is selected. It seems to show ALL the logs for the instance but should only be showing the logs for that particular Plug-in that generated the report. Right?

afbeelding

It seems that it does filter the right hand side logs to the instances, but not for the particular plugin report selected left hand side.

--

Selecting the log text can't go beyond one log entry

The way the view is implemented right hand side for the logs makes it relatively hard to drag select a part of that logs information to paste elsewhere. It'd be great if you could still drag along the text to just copy the raw log text (or that drag selecting would allow only to select full log entries and then copy/pasting that would copy the full text entry so that you could still select and copy paste the text contents.

Missing log type filters for debug, info, warning, errors.

As you mentioned in your notes

Would love to see the buttons again to show/hide specific logging levels again.

Icon versus color circle

I'm not sure what the difference is between the red circle with exclamation point here and the red circle:
afbeelding

I suppose if logs were filtered correctly per plug-in that it might be more obvious that the red exclamation point icon is the actual validation error since it's always the one at the very bottom. The fact that I'm now seeing red circles beneath it I suppose is due to the earlier mentioned bug?


PS. I would love to see this log view widget to find its way into the details panel logs as well but with the additional feature that you can find the source line number (and also see the debug logs which this artist-facing validation report hides by default)

@BigRoy
Copy link
Collaborator

BigRoy commented Apr 29, 2023

Just adding some more screenshots of the current state of the report page in the different situations:

Successful publish

afbeelding
afbeelding

Error publish

afbeelding

Validation Error

afbeelding

openpype/tools/publisher/widgets/report_page.py Outdated Show resolved Hide resolved
openpype/tools/publisher/widgets/report_page.py Outdated Show resolved Hide resolved
openpype/tools/publisher/widgets/report_page.py Outdated Show resolved Hide resolved
openpype/tools/publisher/widgets/report_page.py Outdated Show resolved Hide resolved
openpype/tools/publisher/widgets/report_page.py Outdated Show resolved Hide resolved
openpype/tools/publisher/widgets/report_page.py Outdated Show resolved Hide resolved
@ynbot
Copy link
Contributor

ynbot commented May 9, 2023

Task linked: OP-5711 Add instances to report page

@iLLiCiTiT iLLiCiTiT marked this pull request as ready for review May 10, 2023 18:49
@BigRoy
Copy link
Collaborator

BigRoy commented May 11, 2023

Nice stuff. I pulled the latest with this commit 46efdba

Here are some screenshots, with some questions/notes here and there.

Publish validation error

image

Bug: The actions should not be visible here. They seem to show no matter what I select left hand side.

I personally still feel its weird there's no "Go to detailed logs" button right there. I understand that we might want to avoid artists digging deeper but it just feels weird there's no hint whatsoever as to what's going on.

Publish validation report (failed validations)

image
image

Passed validation

image

Instead of keeping the bottom panel expanded in this case I think it'd be nicer to be consistent and having it still auto-collapse but just highlight the Publish button (maybe making it a clear green color or something that highlights it as "click me"?). It's still just obfuscating the report above it too much.

Or if it was collapsed like this:

image

It could just instead of saying Publish Paused - Validation Passed something like: Validation Passed - Click publish to continue.

Publish successful

image
image

In some cases it shows a gap in the logs, like extra spacing. Here between oiiotool and the Host: fusion log:
image

Is that maybe a trailing \n in the logs? Should we maybe call log.strip() to avoid prefix/trailing new lines?


Other questions

Question 1: How do we set the Context icon? Can we have a global default context icon. It looks a bit empty without it.

Question 2: In the reports I feel the lack of the family headers is a bit of a bummer. I think the design could be more consistent with this design:
image
Instead of this:
image

And then instead of the checkbox/slider it just shows the icon. Thoughts?

@mkolar
Copy link
Member

mkolar commented May 12, 2023

Question 1: How do we set the Context icon? Can we have a global default context icon. It looks a bit empty without it.

I don't see that as a big issue to be honest. yes visually it's empty space, but I wouldn't be that worried about empty spaces. My problem that we even separate context and the workfile even though technically its totally correct, from the user perspective they actually behave quite similarly. I'd almost say that workfile publish should just be an option on the context, in which case we could actually use workfile icon there.

Question 2: In the reports I feel the lack of the family headers is a bit of a bummer. I think the design could be more consistent with this design:

I agree that includes the size of the cards actually.

@mkolar
Copy link
Member

mkolar commented May 12, 2023

Passed validation
Instead of keeping the bottom panel expanded in this case I think it'd be nicer to be consistent and having it still auto-collapse but just highlight the Publish button (maybe making it a clear green color or something that highlights it as "click me"?). It's still just obfuscating the report above it too much.

I can't agree with this. the collapsed states are signifying something is finished, this is mid way through the process though. Also obfuscating the report should not be a problem. It's not there for the artist to browse it, but to give some small context without going to details. What we see in your screen shot is and unfortunate victim of terrible terrible abuse of the logs. Practically all of those should be log.debug which you wouldn't even see here.

The whole point is to only ever present relevant and needed information to the artist, not a overload of developer notes. here the main information is that validations have finished, but publishing has not. Hence the expanded panel appropriate. We should maybe improve the message there though.

@antirotor
Copy link
Member

Not commenting on UI, tested it a little bit in 3dsmax and it behaves. Mainly tested the issue with from pyblish.api import Context and I can confirm that it was fixed.

@kalisp
Copy link
Member

kalisp commented May 15, 2023

In AE and PS: (after Publish in Report tab)

Traceback (most recent call last):
  File "C:\test_build\build\exe.win-amd64-3.9\openpype\lib\events.py", line 195, in process_event
    callback()
  File "C:\test_build\build\exe.win-amd64-3.9\openpype\tools\publisher\widgets\report_page.py", line 1832, in _on_publish_stop
    self._update_state()
  File "C:\test_build\build\exe.win-amd64-3.9\openpype\tools\publisher\widgets\report_page.py", line 1821, in _update_state
    self._publish_instances_widget.update_data()
  File "C:\test_build\build\exe.win-amd64-3.9\openpype\tools\publisher\widgets\report_page.py", line 1714, in update_data
    instance_items = self._get_instance_items()
  File "C:\test_build\build\exe.win-amd64-3.9\openpype\tools\publisher\widgets\report_page.py", line 1691, in _get_instance_items
    instance_items.sort()
  File "C:\test_build\build\exe.win-amd64-3.9\openpype\tools\publisher\widgets\report_page.py", line 646, in __lt__
    values.sort()
TypeError: '<' not supported between instances of 'str' and 'NoneType'

@kalisp
Copy link
Member

kalisp commented May 15, 2023

When validation fails and after Select action is triggered:

Traceback (most recent call last):
  File "C:\test_build\build\exe.win-amd64-3.9\dependencies\pyblish\plugin.py", line 522, in __explicit_process
    runner(*args)
  File "C:\test_build\build\exe.win-amd64-3.9\openpype\hosts\nuke\plugins\publish\validate_asset_name.py", line 47, in process
    self.select(instances)
  File "C:\test_build\build\exe.win-amd64-3.9\openpype\hosts\nuke\plugins\publish\validate_asset_name.py", line 56, in select
    select_node["selected"].setValue(True)
NameError: knob selected does not exist
Traceback (most recent call last):
  File "C:\test_build\build\exe.win-amd64-3.9\openpype\tools\publisher\widgets\report_page.py", line 237, in _on_action_click
    self._controller.run_action(plugin_id, action_id)
  File "C:\test_build\build\exe.win-amd64-3.9\openpype\tools\publisher\control.py", line 2303, in run_action
    self._publish_report.add_action_result(action, result)
  File "C:\test_build\build\exe.win-amd64-3.9\openpype\tools\publisher\control.py", line 286, in add_action_result
    log_items = self._extract_log_items(result)
  File "C:\test_build\build\exe.win-amd64-3.9\openpype\tools\publisher\control.py", line 402, in _extract_log_items
    "is_validation_error": result["is_validation_error"],
KeyError: 'is_validation_error'

@iLLiCiTiT
Copy link
Member Author

@kalisp both issues should be fixed. Please try again, and try to replicate the issues. I'm afraid there will be more issues connected to #4915 (comment) .

@iLLiCiTiT iLLiCiTiT force-pushed the feature/OP-5711_Add-instances-to-report-page branch from 4f2ef2d to d4e7d27 Compare May 17, 2023 12:25
Copy link
Member

@jakubjezek001 jakubjezek001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Redesign of the report page is definitely improving. There is still this last glitch. Not sure why, but this is duplicating the report.
image

@iLLiCiTiT
Copy link
Member Author

Not sure why, but this is duplicating the report

That's because the same message is logged and raised. I can't affect that in UI.

Copy link
Member

@jakubjezek001 jakubjezek001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kalisp
Copy link
Member

kalisp commented May 22, 2023

Adobe hosts are fixed.
Select seems to be fixed too.
(It throws this, but I don't think its a question of this PR.
AnyDeskMSI_DS40Dwevci @jakubjezek001 any idea why is this showing?
)

@BigRoy
Copy link
Collaborator

BigRoy commented May 23, 2023

Did another test run. These are still open notes from before:

  • Report instances on left hand side should look similar to card view left hand side with headers for the families as mentioned here
  • Report instances on left hand side should look similar to card view left hand side in instance list, matching the card size as mentioned here
  • Some logs seems to have redundant extra spacing to the next entry as mentioned here

Examples of the extra spacing:
image
image
image

@iLLiCiTiT iLLiCiTiT force-pushed the feature/OP-5711_Add-instances-to-report-page branch from 1ef3e05 to dfd38a3 Compare May 23, 2023 09:55
@iLLiCiTiT
Copy link
Member Author

Some logs seems to have redundant extra spacing to the next entry as mentioned #4915 (comment)

Can you confirm it does not happen anymore?

@BigRoy
Copy link
Collaborator

BigRoy commented May 23, 2023

Can you confirm it does not happen anymore?

Did a new test-run. It seems resolved!

image
image


Issue:
Just noticed that if you do Validate. And then continue the publish afterwards that the bottom bar doesn't seem to auto-collapse again and thus keeps showing like this (with the wrong sub-header with 'click play to continue'):
image


Also, there is now header separation for the instances in the report on the left. But it doesn't look the same as the "publish" list of instances. The headers are slightly different, etc. I feel like more consistency would be nice, even though this is a decent first pass. @mkolar what do you think?

image

image

Minor side note: isn't "click" a more used term for clicking a button instead of "hit"? Should we change that label?

@iLLiCiTiT
Copy link
Member Author

Also, there is now header separation for the instances in the report on the left. But it doesn't look the same as the "publish" list of instances.

Create page is using create plugin to get the group name. This page is based only on pyblish instance data, not sure if it's worth to try to match them as that would require much more data in report and there would have to be some "default behavior" if it would not be possible to get it.

@iLLiCiTiT
Copy link
Member Author

iLLiCiTiT commented May 23, 2023

Just noticed that if you do Validate. And then continue the publish afterwards that the bottom bar doesn't seem to auto-collapse again and thus keeps showing like this (with the wrong sub-header with 'click play to continue'):

It was already explained. This is by design. It is auto-collapsed only if user can't continue in publishing.

@iLLiCiTiT iLLiCiTiT merged commit a73d19b into develop May 23, 2023
1 check passed
@iLLiCiTiT iLLiCiTiT deleted the feature/OP-5711_Add-instances-to-report-page branch May 23, 2023 16:16
@ynbot ynbot added this to the next-patch milestone May 23, 2023
@BigRoy
Copy link
Collaborator

BigRoy commented May 23, 2023

It was already explained. This is by design. It is auto-collapsed only if user can't continue in publishing.

No, you misunderstand. It remains open EVEN if you click continue. :) So from the paused state you click "publish" and at that point it doesn't collapse.

@iLLiCiTiT
Copy link
Member Author

No, you misunderstand. It remains open EVEN if you click continue. :) So from the paused state you click "publish" and at that point it doesn't collapse.

Not sure I understand? It should not collapse at the moment you click to continue but when it stops...
To auto-collapse must be met 2 conditions:

  1. Publishing has stopped.
  2. User cannot continue in publishing.

I couldn't replicate any case when these conditions are met and it would not collapse.

@BigRoy
Copy link
Collaborator

BigRoy commented May 23, 2023

Ok, so you are correct but there's still a cosmetic bug which is what triggered the confusion for me in the first place.

Yes it should be expanded, but the subtitle/description is incorrect.

Notice in this screenshot that it's currently actively publishing (it's not paused, it's running) but the description says: Hit publish (play button) to continue). Which is incorrect.
image

To reproduce:

  1. Click validate
  2. Wait for it to finish validation successfully
  3. Click publish
  4. Now while it is publishing it will be expanded (that's correct) however the subtitle (second line in the message) remains Hit publish (play button) to continue) which is confusing, because it's currently playing/publishing.

This can also be reproduced by:

  1. Click validate
  2. Stop/Pause the validation
  3. Continue the validation or publishing
  4. Now while it is continuing it'll keep the subtitle "click publish to continue" which is confusing.

Expected behavior

The message should've been cleared as soon as "publish" was clicked after the paused validation.


Also, another side note. Whenever an instance has no active info logs it looks like this:

image

I wonder whether that's the way we want it to look cosmetically. It's nothing criticial, just bringing it up.

@iLLiCiTiT
Copy link
Member Author

Notice in this screenshot that it's currently actively publishing (it's not paused, it's running) but the description says: Hit publish (play button) to continue). Which is incorrect.

Created task for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XL Denotes a PR changes 1500-2499 lines, ignoring general files tool: Publisher type: feature Larger, user affecting changes and completely new things
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

8 participants