-
Notifications
You must be signed in to change notification settings - Fork 2
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
Rework HTML slate renderer. #27
Conversation
I've been following this a bit and just wanted to chime in on something that was marked to change: "Renderer will now raise a ValueError if the provided data does not fill up all of the expected template field (instead of silently hidding them)" The way we thought about it is that whenever there is a property that is not resolved we sub that prop with a specific string. That string becomes an identifier that is able to then hide the parent container if the info is not present. This is due to the fact that sometimes clients ask for very informative slates, but not always do we have data to show.
If raising ValueError could be default behaviour but could be also opted out in favor of a workflow like that it would be most helpful. We do rely on a mechanism like that to hide and make the slates not crash most of the time in house. As its is you are making the assumption that whoever fills the data
which is never the case in our experience, and Raising generates a lot of requests for pipe dept to fix things asap :) |
Thanks a lot @maxpareschi for the user/experience feedback, definitely most valuable. All things considered, maybe I should actually go ahead and implement it right now. EDIT: Just added a |
tests/test_renderers_slate.py
Outdated
yield temp_dir | ||
|
||
# Remove all temporary directory content. | ||
shutil.rmtree(temp_dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets implement the fixture from tests\conftest.py
for test_staging_dir please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it would be great for later test result analyses if each of test case would output into its own folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be sure this is what you had in mind.
I've adjusted this fixture to leverage the conftest.py/test_staging_dir
one, for each test it will create a dedicated folder specific for this test and copy the resource sequence files over (so we can test the default prepend slate destination behavior). At the end of the test session test_staging_dir
is deleted which cleans everything.
This way we ensure each test result is scoped .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure where is this coming from but my results are produced following results.
Thanks for the material, this is very puzzling... Is this happening for all tests or just this one ? From the screenshot, you can see:
I had a look at the selenium reported issues on the repo but I could not find anything meaningful. Out of curiosity, would it be please possible to run the test on your side and confirm which values you obtain for those ? In the meantime, I have also added an additional Honestly, this looks more a patch than a proper required thing here.. but I couldn't think of anything better. @tweak-wtf by any chance, would you also be able to please run the tests on this branch on your machine ? Would help a lot. |
OT: i probably should set something up where we can download latest produced test result from the GH actions. could be |
defaulting to 1920x1080 feels fine to me. |
Here are the resolution results I printed from line 256 in
The resulting resolutions seem to be off. In the example image, the output from test_Slaterenderer_default should be 1920x1080, but it's coming out as 2459x1859. There's also an issue with the broken template. This might be caused by the miscalculation, which then affects other calculations. Here is the original template look and the resulting look. |
Just to tell you that chrome in the —headless=new mode has two “bugs” (which apparently they are intended behavior but still)
|
Co-authored-by: Jakub Ježek <[email protected]>
Thanks a lot @maxpareschi, thanks to you I think I found something. I played a bit with the Hight DPI / scale options in Windows and could reproduce a resolution mismatch (see scale 150%). I've managed to fix this introducing a To me, even with 125% or 150% set, the resulting HD and 4K slates matches the expected resolution and visual ( @jakubjezek001 does it fix the issue on your end as well ? |
@@ -73,11 +130,12 @@ def render(self) -> None: | |||
f"""'{timecode.replace('"', "")}'""", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jop the timecode issue i noted is only spotted now since spending some love on the slates.
played around a bit and for me this simple refactor actually did the trick lol.
f"""'{timecode.replace('"', "")}'""", | |
timecode, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm that this is fixing the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and all works fine now. @robin-ynput perfect!
Yeah the timecode issue is its own PR scope @tweak-wtf what do you thing. But could be added here if you found it important to include it.
cococool! i do think it's important that the TC is consistent but yeah probably a different PR. lemme go ahead on this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's gooo 🚀
Changelog Description
Addresses the TODOs for HTML slate renderer:
Changed behavior:
ValueError
by default if the provided data does not fill up all of the expected template fieldSlateFillMode.HIDE_FIELD_WHEN_MISSING
moderesolution
,date
and inputframe range
Additional info
Contributes to #26
Depends on #30
Testing notes:
tests/test_renderers_slate.py
.\start.ps1 test
(Windows only)