Refactor alphablit #3404
Refactor alphablit #3404
Conversation
MightyJosip
added
the
Code quality/robustness
|
Hi JoKing, this PR is a good modernization effort idea. One thing I've learned in my experience with macros in the pygame-ce codebase is that it's nice when the function names are not macro-ed, because then you can use your editor to jump to them or just search for them. Like after this PR if I want to go see what This PR also has a merge conflict at this point. |
|
Thanks. Yea I agree with you, makes sense, I will change it a bit |
There was a problem hiding this comment.
This looks good, thanks @MightyJosip.
I made a couple enhancements to the implementation in my own 2 commits, I figured it was just easier to commit what I had in mind than send the idea over to you. But I simplified the macros a bit (they don't need to be written like function calls, temp variables can be declared outside), and I also renamed the arguments to be more descriptive and match macro naming conventions (all caps). If you disagree with those changes feel free to revert my commits (it's your branch after all).
|
Yea sure, thanks for helping, the commit is better now |
There was a problem hiding this comment.
+470 and -1725, I love seeing PRs with huge negative diffs 😄
great cleanups and duplication removals, I did a side by side compare of alphablit.c after expanding macros and didn't find any issues (infact I think this PR fixes a minor issue, there were duplicated SDL_MapRGB calls before this PR in some places)
Current alphablit has way too many repetitive code. For example, in each blend blitter, around 90% of the code is the same. So I decided to move it into the macro, and by that, reduce the file by around 40% of the lines.
The good side of this commit is that if we do eventual change on the blitter code, we only need to do it in the one place, the code became more modular. However, macros probably make the code harder to read, so we need to decide will we proceed with this change