Skip to content

Add screenshot 1342 - #2039

Closed
sabadam32 wants to merge 46 commits into
pythonarcade:developmentfrom
sabadam32:add_screenshot_1342
Closed

Add screenshot 1342#2039
sabadam32 wants to merge 46 commits into
pythonarcade:developmentfrom
sabadam32:add_screenshot_1342

Conversation

@sabadam32

@sabadam32sabadam32 commented Mar 29, 2024

Copy link
Copy Markdown
Contributor

Fixes#1342

@pushfoopushfoo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I can't speaks to the GL side, but there are some immediate changes I see as room for improvement. Included file in here + some possible improvements.

Comment threadtash apply Outdated
Comment threadarcade/application.py Outdated
Comment threadarcade/window_commands.py Outdated
Comment threadtash apply Outdated
Comment threadarcade/application.py Outdated
Comment threadtests/unit/window/test_screenshot.py
@einarf

Copy link
Copy Markdown
Member

What about the old arcade.get_image() and arcade.get_pixel()? We also talked about having screenshot feature in cameras. I think get_image() also have some history related to pixel ratio > 1.0. I don't quite remeber if we ended up scaling the screenshot itself.

@pushfoo

Copy link
Copy Markdown
Member

also have some history related to pixel ratio > 1.0

Is this HiDPI or strange edge cases where non-square pixels exist? To my knowledge, that only exists on ancient platforms, but I figured I should double check.

pushfooand others added 13 commits March 31, 2024 11:18
* Remove extra sentence
* Add list item linking the better datetime overview
* Re-order arguments for get_timestamp
* Improve default timestamp format string
* Add time-machine test helper to pyproject.toml
* Add tests for get_timestamp
* Update doc to cover changes
* Add %Z to get_timestamp
* Add doc on it + cross-refs
* Clean datetime test imports

@pushfoopushfoo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TL;DR: _TzInfo is protected and ugly, but so was my previous PR due to pyright breakage. Sorry about that.

I'd run a local pyright check on the changes I proposed. My pyright is broken because of nodejs issues on my distro, sadly. Sorry about my previous PR being a little broken because of it.

Comment threadarcade/__init__.py Outdated

def get_timestamp(
how: str = "%Y_%m_%d_%H%M_%S_%f%Z",
when: Optional[types.HasStrftime | datetime] = None,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  1. Did HasStrftime not match datetime? Was it only pyright which complained?
  2. To my understanding, the | syntax is a 3.9+ feature, but we support 3.8+

@sabadam32sabadam32Apr 6, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

No it didn't. pyright complained for sure, but I don't remember if the build failed due to this check

@pushfoopushfooApr 6, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are you sure? I see references to line 41 in the GitHub CI builds on the previous commits. That's the other line it seems.

Comment threadarcade/__init__.py Outdated
import sys
import os
from datetime import datetime
from datetime import datetime, _TzInfo

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My initial PR might have been a little busted due to my local nodejs breaking my pyright (I know pyright shouldn't depend on node, but it does 😢). This should work (I think):

Suggested change
fromdatetimeimportdatetime, _TzInfo
fromdatetimeimportdatetime, tzinfo

The Python source for the datetime module shows it as a real ABC, and it should be less brittle than _TZInfo.

Comment threadarcade/__init__.py Outdated
when: Optional[types.HasStrftime] = None,
tzinfo: Optional[datetime.tzinfo] = None
when: Optional[types.HasStrftime | datetime] = None,
tzinfo: Optional[_TzInfo] = None

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think I removed a top-level datetime import but left behind the broken property datetime.tzinfo line earlier. With the import suggestion at the top, I think this should work.

Suggested change
tzinfo: Optional[_TzInfo] =None
tzinfo: Optional[tzinfo] =None

@pvcraven

Copy link
Copy Markdown
Member

I like having the additional docs.

Although, in the past, when people have wanted to do a screenshot we've just pointed them to:

image=get_image()
image.save('screenshot.png', 'PNG')

This seems like a lot to add to the library for screenshots when two-lines can also run a screenshot.

If we do add it, can we get some unit tests?

@pushfoo

pushfoo commented Apr 11, 2024

Copy link
Copy Markdown
Member

TL;DR:

  1. We have some unit tests already
  2. It seems since it's all at once instead of over 3+ years
  3. save_image is clear and discoverable

The Precedent is 2-3 Feature Exposures

This seems like a lot to add to the library for screenshots

In the past, we slowly added features to the top-level namespace, Window, and View. Two of the top examples:

FeatureWindowViewTop-level
ClearingWindow.clear()View.clear()arcade.start_render() (arcade.get_window.clear())
Setting BG colorWindow.background_colorView.background_colorarcade.set_background_color

Why Do It For Screenshots?

  1. View is expected to share core API features with Window
  2. arcade.get_image sets the precedent for exposing screenshots at the top level

Unit Tests

If we do add it, can we get some unit tests?

Can you elaborate? We already have some here:

If you'd like, we add some to explicitly cover View.

@einarfeinarf added this to the Future milestone Jun 15, 2024
@einarf

einarf commented Jun 27, 2024

Copy link
Copy Markdown
Member

We can revisit this in 3.x. Let's keep this PR around.

There's things like docs in there that can be salvaged.

@DragonMoffon
DragonMoffon marked this pull request as draft September 29, 2024 02:04
@sabadam32sabadam32 closed this by deleting the head repository Feb 24, 2025
Sign up for freeto 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.

Build improved screenshot API

5 participants

@sabadam32@einarf@pushfoo@pvcraven@rich-saupe