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

Use cython fused types (templated functions) to remove redundancy from smp.pyx and smx.pyx #1370

Open
TheJJ opened this issue Jan 21, 2021 · 2 comments · May be fixed by #1735
Open

Use cython fused types (templated functions) to remove redundancy from smp.pyx and smx.pyx #1370

TheJJ opened this issue Jan 21, 2021 · 2 comments · May be fixed by #1735
Labels
area: assets Involved with assets (images, sounds, ...) good first issue Suitable for newcomers improvement Enhancement of an existing component lang: cython Implemented using Cython code

Comments

@TheJJ
Copy link
Member

TheJJ commented Jan 21, 2021

Required skills: Cython

Difficulty: Medium

SMX is the graphics storage format in AoE2:DE. We export these files to PNG in the converter.

Because of the different storage formats and layers, the process_drawing_cmds function exists 3 times in smp.pyx, and 5 times in smx.pyx, both in openage/convert/value_object/read/media/.
We can use one generic function (one for smp, one for smx) that can handle all of the features at once.

We can do that using templates in Cython, called "fused functions".

Here's an example how it works in principle.

  • The rolf function is saved two times - once for variant_1 and once for variant_2. And it won't have any if in its body then.
  • Thus these if CompressionVariant is lines behave like if constexpr inside a templated entity.
cdef class variant_1:
    pass
cdef class variant_2:
    pass

cdef fused CompressionVariant:
    variant_1
    variant_2

cdef rolf(CompressionVariant a):
    print("common!")
    if CompressionVariant is variant_1:
        print("variant 1")          
    elif CompressionVariant is variant_2: 
        print("variant 2") 
    else:
        print('wtf?')   


cdef main():
    print("run")
    cdef variant_1 v1 = variant_1()
    cdef variant_2 v2 = variant_2()
    rolf(v1)    
    rolf(v2)      

main()

We defined multiple variants of process_drawing_cmds to avoid a lot of branches (ifs) in the codepath, since this function is called more than a million times during the conversion process.

You can test your changes with the singlefile media converter.

Further reading:

@TheJJ TheJJ added improvement Enhancement of an existing component lang: python Done in Python code area: assets Involved with assets (images, sounds, ...) good first issue Suitable for newcomers labels Jan 21, 2021
@heinezen heinezen added the hacktoberfest For newcomers from Hacktoberfest event label Sep 30, 2021
@heinezen heinezen removed the hacktoberfest For newcomers from Hacktoberfest event label Nov 9, 2021
@Sujal-Rana-88

This comment was marked as spam.

@heinezen heinezen added lang: cython Implemented using Cython code and removed lang: python Done in Python code labels Sep 8, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in openage converter Sep 10, 2023
@heinezen heinezen moved this from 📋 Backlog to 🔖 TODO in openage converter Sep 10, 2023
@heinezen heinezen moved this to converter in openage beginner tasks Sep 10, 2023
@namka279

This comment has been minimized.

@heinezen heinezen added the hacktoberfest For newcomers from Hacktoberfest event label Sep 20, 2024
@heinezen heinezen removed the hacktoberfest For newcomers from Hacktoberfest event label Dec 7, 2024
@jere8184 jere8184 linked a pull request Dec 27, 2024 that will close this issue
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, ...) good first issue Suitable for newcomers improvement Enhancement of an existing component lang: cython Implemented using Cython code
Projects
Status: converter
Status: 🔖 TODO
Development

Successfully merging a pull request may close this issue.

4 participants