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.

Sign up for GitHub

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

Jump to bottom

Type consolidation and improvements #5319

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

Type consolidation and improvements #5319

wants to merge 13 commits into from

Conversation

mal
Copy link
Member

@mal mal commented Jan 29, 2024 •
edited

Recommend commit-by-commit review using commit messages as your guide.

Headlines:

  • Moved absolute, position, and DualAngle types to new renpy.display.types module, along with various coercion functions.
  • Optimised creation of position type negating need for position.from_any.
  • Added support for == and != operators to position to aid transform change detection code.
  • Removal of position_or_none in favour of allowing position to gracefully handle None itself.
  • Moved DualAngle.from_any to module level dualangle and optimise type checks.
  • Moved absolute.compute_raw to 'absolute.compute' and optimised type checks.

Commit messages have a little more detail, but there's anything you feel requires more explanation give me a shout. :)

@mal mal force-pushed the type-improvements branch 2 times, most recently from 9874ccd to 26551fe Compare January 29, 2024 11:21
@renpytom
Copy link
Member

renpytom commented Jan 29, 2024 •
edited

My first reaction is that I really do not like the renaming of absolute.compute_raw to pixels. To me, the name "pixels" refers to an array of pixels, and not a position. 200709d should go away - it seems more confusing to me. (Or perhaps become absolute with a second parameter.)

I'm not in love with compute_raw, either, but I think 'pixels' is worse.

@renpytom
Copy link
Member

eb05eda - I like the idea of this, but I think it should probably be renpy.display.types, since it only has display-related types.

aeb4586 - This seems wrong. If someone was to write:

position(absolute=0.5)

They'd get position with relative set to 0.5, which is very confusing.

eae1b6d - Fine.

16be044 - Fine.

cba2c29 - Fine.

c15f39c - Seems unsafe, because of aeb4586.

09490b7 - As we discussed on dev, having a type's constructor return an instance of a different type is very confusing. I like position_or_none, since it matches the way I think about positions.

8d82878 - Fine.

200709d - As discussed, I really don't like the name pixels. I do like absolute, though I'm not a huge fan of absolute.compute_raw as a name.

I'm wondering if it makes sense to replace absolute.compute_raw with absolute.compute, and speed absolute.compute up so we can just used it everywhere.

26551fe - Fine.

ba1bf44 - Fine, but we may not want to do this everywhere, only in cases like this where it's really obvious what's absolute and what's relative.

@mal
Move types and coercion functions to own module
ad3c60d 
The content of the new types module is purely lift and shift, with the
exception of references to other moved types. Some functions and methods
have been reordered but the code is unchanged.
@mal
pos: Eliminate round-trips from position coercion
509d3a9
@mal
pos: Add explicit operator methods to position
2249826 
This avoids other code needing to know or care about which operators are
implemented directly and thus more performant and which are amalgams of
other operators with additional overhead.
@mal
pos: Add support for equality checks to position
8e43693 
This is used by the transform interpolation code to identify which
properties have changed. The lack of this method causes it to run no-op
interpolations.
@mal
pos: Use local references to save lookups
f5c7724
@mal
pos: Remove obsolete from_any method
69cb6b8
@mal
pos: Drop ATL *_or_none functions
6a570b4 
The rationale for this is that they didn't serve their intended purpose,
and somewhat misleading. They only called by interpolation, and in such
a way that the argument could never be None.

In 8.2 a special case was added to spline interpolation which used one
of these functions directly, however the additional None handling failed
to make the result any more usable. In order to address this in a more
useful way, and align with non-spline interpolation, None values will
now be treated as zero.
@mal
Expand dual angle operators and separate from_any
9a76213 
Same logic as for position, other code shouldn't have to worry which
operators have first class support, and there are less mental hoops to
jump through when we look back from years in the future.
@mal
Apply position type pattern to dualangle
d82f9da 
Merges the DualAngle type and dualangle function removing the burden on
calling code to appreciate the nuance and simply ask for a dualangle.
@mal
abs: Rename compute_raw to compute on absolute
18d32aa
@mal
Minor style tweaks
82bcbc5
@mal
Clean up redundant use of keyword args
e824522
@mal
Copy link
Member Author

mal commented Feb 13, 2024

aeb4586 - This seems wrong. If someone was to write:

position(absolute=0.5)

They'd get position with relative set to 0.5, which is very confusing.

This is addressed by making the first argument of position positional and thus required, eliminating this possibility. The public API docs remain unchanged, and still document that two positional parameters is the only supported signature.

c15f39c - Seems unsafe, because of aeb4586.

Covered by the adjustment above.

09490b7 - As we discussed on dev, having a type's constructor return an instance of a different type is very confusing. I like position_or_none, since it matches the way I think about positions.

This has been replaced with 6a570b4, which seeks to simplify the ATL property type spec to what is functionally useful (see commit message for more details on why the *_or_none pseudo-types weren't really used/very useful.

200709d - As discussed, I really don't like the name pixels. I do like absolute, though I'm not a huge fan of absolute.compute_raw as a name.

I'm wondering if it makes sense to replace absolute.compute_raw with absolute.compute, and speed absolute.compute up so we can just used it everywhere.

This has been done, moving to and optimising absolute.compute instead.

ba1bf44 - Fine, but we may not want to do this everywhere, only in cases like this where it's really obvious what's absolute and what's relative.

The calling sites affected by this use variable names making it clear which is which, so I think this is fine as long as we're conscious of the same concern when it comes to any future uses.

Additional changes include:

  • d82f9da
    Merged DualAngle and dualangle resulting in fewer imports, simplification of calling code that no longer needs to worry about which capitalisation variant is required, and mirrors the behaviour of the position type - such that groking one is groking both.

  • 1e0b9bb
    Properly support pickling of objects using __new__ by adding __getnewargs__, and adding unpickling compat for 8.2 types.

I don't love the pickle compat, but after trying multiple approaches this was the one that worked, was the most sane, and didn't add confusion to other parts of the repo. I have the beginnings of an idea for general improvements to inter-version pickle compat, however they are out of scope for this PR.

@mal
Ensure pickle safety for position and dualangle
f6bf7f9 
We got away without defining __getnewargs__ in 8.2 because we didn't
require arguments to __new__, but probably still ought to have done it.

As pickled 8.2 types don't have the necessary arguments available to
them, they require compat wrappers during unpickling.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mal @renpytom