[WIP] Use template functions for SMX and SMP file #1427
Conversation
|
It's probably easier to discuss most changes with code that compiles properly, but I already have some comments :) |
|
|
||
|
|
||
| cdef fused ProcessDrawingCmdsVariant: | ||
| SMXLayerVariant |
There was a problem hiding this comment.
You can call the fuse type SMXLayerVariant instead of ProcessDrawingCmdsVariant. SMXLayerVariant is never used anyway.
| if ProcessDrawingCmdsVariant is SMXLayerVariant: | ||
| pass |
| if ProcessDrawingCmdsVariant is SMXMainLayer8to5Variant: | ||
| elif lower_crumb == 0b00000010: |
| cdef uint8_t palette_section_block = 0 | ||
| cdef uint8_t palette_section = 0 | ||
|
|
||
| if ProcessDrawingCmdsVariant is SMXShadowLayerVariant || ProcessDrawingCmdsVariant is SMXOutlineLayerVariant: |
There was a problem hiding this comment.
It's better to use the Pythonic style to write this line:
if ProcessDrawingCmdsVariant in (SMXShadowLayerVariant, SMXOutlineLayerVariant):
And || is not a Python operator. Better use or.
| uint8_t damage_modifier_2 # modifier for damage (part 2) | ||
|
|
||
|
|
||
| cdef fused ProcessDrawingCmdsVariant: |
There was a problem hiding this comment.
You also need to declare each variant (SMXMainLayer8to5Variant, SMXMainLayer4plus1Variant, SMXOutlineLayerVariant, SMXShadowLayerVariant) with cdef class ... like in @TheJJ 's example in the issue.
| if ProcessDrawingCmdsVariant is SMXMainLayer4plus1Variant: | ||
| elif lower_crumb == 0b00000010: |
| # Process next command | ||
| dpos_cmd += 1 | ||
| if ProcessDrawingCmdsVariant is SMXMainLayer8to5Variant || ProcessDrawingCmdsVariant is SMXMainLayer4plus1Variant: | ||
| return dpos_cmd, dpos_color, odd, row_data | ||
| if ProcessDrawingCmdsVariant is SMXOutlineLayerVariant || ProcessDrawingCmdsVariant is SMXShadowLayerVariant: | ||
| return dpos_cmd, dpos_cmd, chunk_pos, row_data |
There was a problem hiding this comment.
The indentation is really weird here.
|
My recommendation would be to compare the similar-but-not-identical functions side by side (or copy them to files, and do |
|
Additionally, it might be beneficial to use the profiling utilities to compare the speed of the implementation before and after each change. |
|
@9a24f0 Do you need help with completing the PR? |
This comment was marked as spam.
This comment was marked as spam.
|
Implemented by #1735 |
Hi, I opened this PR to keep track on my work and to receive some reviews.
@heinezen and @TheJJ I really need some walk from you two to complete GH-1370.