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

Merge Render.blit, absolute_blit and relative_blit #4427

Open
Gouvernathor opened this issue Mar 8, 2023 · 4 comments · May be fixed by #4430
Open

Merge Render.blit, absolute_blit and relative_blit #4427

Gouvernathor opened this issue Mar 8, 2023 · 4 comments · May be fixed by #4430
Labels
task Something that isn't an issue or an enhancement, but still needs to be done.

Comments

@Gouvernathor
Copy link
Member

Gouvernathor commented Mar 8, 2023

Given how the methods are used in internal code, I propose the following architecture.

  • _blit or raw_blit - implements the behavior common to all three former methods, without doing anything to the type of pos
  • integer_blit or iblit - does what the current blit does : rounds (down ?) the pos values to integers and passes them to _blit. Used instead of blit in existing internal code.
  • absolute_blit - does the same as the former absolute_blit and subpixel_blit, kept for compatibility with internal code.
  • blit - exposed as part of the public CDD Render API for creators to use, does a compated typechech on the pos values : if the compat disables it, calls integer_blit, otherwise treats them as positions (multiply pure floats with the size of the Render) and pass it to _blit.

Feedback on this is appreciated.

@Gouvernathor Gouvernathor added the task Something that isn't an issue or an enhancement, but still needs to be done. label Mar 8, 2023
@Gouvernathor Gouvernathor linked a pull request Mar 8, 2023 that will close this issue
@renpytom
Copy link
Member

renpytom commented Mar 9, 2023

I'm not sure what the benefit of a change that goes from three blit methods to three blit methods is. It seems like we'd be doing a lot of work for not a lot of improvement.

@Gouvernathor
Copy link
Member Author

The benefits would be twofold : one, clarify what function does what, two, add the position behavior to the public method.
Plus, absolute_blit and integer_blit would only be kept for compatibility for code written without caring for types, and would not be used in new internal code.

What's more, I suspect the functions called by these blit methods don't actually care about the type of the values, and only consider them as absolute number of pixels. If that's confirmed, then all three existing methods actually do the same thing and could be merged into one raw_blit passing the values without conversion. Another creator-public blit that would implement the position mechanism could then be added aside it, and we would end up with only two methods.

@mal
Copy link
Member

mal commented Mar 9, 2023

The proposed changes add additional lookups, conditionals and/or frames to the stack for both blit and subpixel_blit. Given the frequency with which these methods are called the small amount of existing code duplication seems much preferable.

@Gouvernathor
Copy link
Member Author

The second version with raw_blit would actually quicken the code for a lot of uses (if not all) where the type of pos doesn't matter - and look it up, very often blit is called with literal (0, 0). Calling absolute on the pos when needed then calling raw_blit, instead of calling absolute_blit, would be equivalent in terms of call time.
In that case, nested calls would be reserved for internal uses where we don't bother to figure out the types (but that can be phased out), and only if it turns out that the type of the values actually matter, which I do't think it does. The creator-public method implements more features, so it makes sense for it to be a bit slower.

I think we're a bit in the fog until we know for sure whether passing an int or a float/absolute with an integer value actually makes any difference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
task Something that isn't an issue or an enhancement, but still needs to be done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants