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

Decouple src/math interface from libultra #62

Open
GiacomoGarbin opened this issue Apr 28, 2024 · 3 comments
Open

Decouple src/math interface from libultra #62

GiacomoGarbin opened this issue Apr 28, 2024 · 3 comments
Labels
refactoring Structural changes to code

Comments

@GiacomoGarbin
Copy link
Contributor

GiacomoGarbin commented Apr 28, 2024

Only two header files need refactoring, both of which reference libultra's Mtx:

  • matrix.h: matrixFromBasisL is only referenced once in debug_render.c, so for simplicity I would move the function definition there and remove the function declaration from matrix.h.

  • transform.h: transformToMatrixL is referenced by multiple files, so I thought of moving the library-dependent interface to a new file transform_libultra.h.

    transform.h

    // library independent-interface
    

    transform_libultra.h

    #include “transform.h”
    #include <ultra64.h>
    
    // transformToMatrixL declaration
    

    transform_libultra.c

    #include “transform_libultra.h”
    
    // transformToMatrixL definition
    

    e.g. portal_gun.c

    #include “transform_libultra.h”
    
    // call to transformToMatrixL
    
@mwpenny mwpenny added the refactoring Structural changes to code label Apr 30, 2024
@mwpenny
Copy link
Owner

mwpenny commented Apr 30, 2024

Your suggestion for matrixFromBasisL() is pretty easy and cleans things up a bit.

For transformToMatrixL(), as you noted, it is used throughout the codebase for rendering. The libdragon renderer will likely have many differences from the libultra-based implementation - unlike the controller code - and so I think that a better point of abstraction in this case is rendering itself, rather than the various functions called by it (which may not be used in both cases). Focusing on system as a whole instead of its inner-workings.

This is quite a large area and will likely be the most complex to port, so it's better left until we make some progress on the other areas and have some libdragon integration (which will also give a better understanding of the different abstraction approaches and pitfalls). So doing it now feels a bit premature.

I appreciate you being proactive here. As for your suggestion on how to split up the functions, we'll likely need to have a mix of common and library-specific functions in the same logical unit at some point - rendering or not. In that case, either splitting it like you're suggesting or having a conditionally-included header with some typedefs would work well, depending on the situation.

@GiacomoGarbin
Copy link
Contributor Author

Your suggestion for matrixFromBasisL() is pretty easy and cleans things up a bit.

Ok, I'll open a PR for this easy change.

For transformToMatrixL(), as you noted, it is used throughout the codebase for rendering. The libdragon renderer will likely have many differences from the libultra-based implementation - unlike the controller code - and so I think that a better point of abstraction in this case is rendering itself, rather than the various functions called by it (which may not be used in both cases). Focusing on system as a whole instead of its inner-workings.

This is quite a large area and will likely be the most complex to port, so it's better left until we make some progress on the other areas and have some libdragon integration (which will also give a better understanding of the different abstraction approaches and pitfalls). So doing it now feels a bit premature.

I see your point and I agree. transform.h, however, seems to be general enough that both libultra and libdragon implementations (and others as well) can rely on it: it seems that the Transform structure encapsulates the data for classical 3D transformations, so transform.h looks more like a 3D math module rather than a libultra rendering support module. But as you said, this is just a premature assumption and there is no harm in postponing any refactoring required here until we have a better overall picture.

@mwpenny
Copy link
Owner

mwpenny commented May 4, 2024

You're correct that the transform functions are mostly general purpose. I didn't mean that the module itself is for rendering. Rather, transformToMatrixL() is used for rendering. It converts the generic Transform struct used by the other functions into libultra's fixed-point Mtx.

It's possible there will be something similar required for libdragon, but it's fairly low level this early on.

Also, glad to hear you mention non-libultra and non-libdragon backends. I had the same thought long-term :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Structural changes to code
Projects
None yet
Development

No branches or pull requests

2 participants