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

[WIP] Use template functions for SMX and SMP file #1427

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

9a24f0
Copy link

@9a24f0 9a24f0 commented Oct 25, 2021

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.

@9a24f0 9a24f0 marked this pull request as draft October 25, 2021 17:34
@heinezen heinezen added area: assets Involved with assets (images, sounds, ...) hacktoberfest For newcomers from Hacktoberfest event improvement Enhancement of an existing component lang: python Done in Python code labels Oct 27, 2021
@heinezen
Copy link
Member

It's probably easier to discuss most changes with code that compiles properly, but I already have some comments :)

@@ -54,6 +54,313 @@ cdef struct pixel:
uint8_t damage_modifier_2 # modifier for damage (part 2)


cdef fused ProcessDrawingCmdsVariant:
SMXLayerVariant
Copy link
Member

Choose a reason for hiding this comment

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

You can call the fuse type SMXLayerVariant instead of ProcessDrawingCmdsVariant. SMXLayerVariant is never used anyway.

Comment on lines 77 to 78
if ProcessDrawingCmdsVariant is SMXLayerVariant:
pass
Copy link
Member

Choose a reason for hiding this comment

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

This does nothing.

Comment on lines 263 to 264
if ProcessDrawingCmdsVariant is SMXMainLayer8to5Variant:
elif lower_crumb == 0b00000010:
Copy link
Member

Choose a reason for hiding this comment

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

Wrong order of lines?

cdef uint8_t palette_section_block = 0
cdef uint8_t palette_section = 0

if ProcessDrawingCmdsVariant is SMXShadowLayerVariant || ProcessDrawingCmdsVariant is SMXOutlineLayerVariant:
Copy link
Member

Choose a reason for hiding this comment

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

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.

@@ -54,6 +54,313 @@ cdef struct pixel:
uint8_t damage_modifier_2 # modifier for damage (part 2)


cdef fused ProcessDrawingCmdsVariant:
Copy link
Member

Choose a reason for hiding this comment

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

ctypedef instead of cdef

Copy link
Member

Choose a reason for hiding this comment

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

You also need to declare each variant (SMXMainLayer8to5Variant, SMXMainLayer4plus1Variant, SMXOutlineLayerVariant, SMXShadowLayerVariant) with cdef class ... like in @TheJJ 's example in the issue.

Comment on lines 321 to 322
if ProcessDrawingCmdsVariant is SMXMainLayer4plus1Variant:
elif lower_crumb == 0b00000010:
Copy link
Member

Choose a reason for hiding this comment

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

Wrong order of lines?

Comment on lines 356 to 375
# 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
Copy link
Member

Choose a reason for hiding this comment

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

The indentation is really weird here.

@TheJJ
Copy link
Member

TheJJ commented Oct 27, 2021

My recommendation would be to compare the similar-but-not-identical functions side by side (or copy them to files, and do diff, ...). Then you know what the differences are and copy they into one bigger function, where you enable/disable these differences through the fused-type-ifs.

@heinezen
Copy link
Member

Additionally, it might be beneficial to use the profiling utilities to compare the speed of the implementation before and after each change.

@heinezen
Copy link
Member

heinezen commented Nov 3, 2021

@9a24f0 Do you need help with completing the PR?

@namka279

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: assets Involved with assets (images, sounds, ...) hacktoberfest For newcomers from Hacktoberfest event improvement Enhancement of an existing component lang: python Done in Python code
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

4 participants