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

Breaking Change: Remove 'Callback' from Python host fn method/class names. #997

Merged
merged 3 commits into from
Dec 7, 2022

Conversation

Robadob
Copy link
Member

@Robadob Robadob commented Nov 24, 2022

This better unifies it with the C API.

Also fixed a bug, whereby the __disown__() fix (#975), had failed to address addExitCondition().

Have also made it private LayerDescription::_addHostFunction().

Docs PR: FLAMEGPU/FLAMEGPU2-docs#128

Relates #237 #375

@Robadob Robadob self-assigned this Nov 24, 2022
@Robadob Robadob added the bug label Nov 24, 2022
@Robadob Robadob marked this pull request as ready for review November 24, 2022 15:12
@Robadob Robadob changed the title Breaking Change: Remove "callback" from Python host function related methods/classes. Breaking Change: Remove 'Callback' from Python host fn method/class names. Nov 24, 2022
@Robadob Robadob marked this pull request as draft November 28, 2022 14:58
@Robadob
Copy link
Member Author

Robadob commented Nov 28, 2022

Appears some changes in this PR have not worked as intended and have broken the __disown__() fix (appears the add init/step/exit function methods are now treated as overloads.

e.g.

    def addInitFunction(self, *args):
        r"""
        *Overload 1:*

        Adds an init function to the simulation
        Init functions execute once before the simulation begins
        :type func_p: void
        :param func_p: Pointer to the desired init function
        :raises: exception::InvalidHostFunc If the init function has already been added to this model description
        Notes: Init functions are executed in the order they were added to the model

        |

        *Overload 2:*

        Adds an init function callback to the simulation. The callback objects is similar to adding via addInitFunction
        however the runnable function is encapsulated within an object which permits cross language support in swig.
        Init functions execute once before the simulation begins
        :type func_callback: :py:class:`HostFunctionCallback`
        :param func_callback: Pointer to the desired init function callback
        :raises: exception::InvalidHostFunc If the init function has already been added to this model description
        Notes: Init functions are executed in the order they were added to the model
        """

        try:
            func_callback = func_callback.__disown__()
        except:
            pass


        return _pyflamegpu.ModelDescription_addInitFunction(self, *args)

@Robadob
Copy link
Member Author

Robadob commented Nov 28, 2022

Ah think I've solved the issue. I had ignored the functions incorrectly specifying FLAMEGPU_HOST_FUNCTION_POINTER rather than the more specific init/step/exit aliases used in the header.

Copy link
Member

@ptheywood ptheywood left a comment

Choose a reason for hiding this comment

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

Looks good, pytest all passes under linux.

Raised one comment about a c++ class with Callback in the name still incase it was missed rather than intentionally still there, but its fine if it is intentional.

include/flamegpu/model/LayerDescription.h Show resolved Hide resolved
Robadob and others added 3 commits December 7, 2022 15:13
…ames

BugFix: #975 had failed to address addExitCondition()
Most appropriate tests have been ported from the C test suite, some have been removed where Python implementation of vector differs (e.g. begin()/insert() etc).
Closes #557
@Robadob
Copy link
Member Author

Robadob commented Dec 7, 2022

Assuming CI passes this can be merged.

There's something I don't think should compile (a returned struct before it's used by a prototype, so size unknown), but it worked for me locally.

@Robadob Robadob merged commit 86ba187 into master Dec 7, 2022
@Robadob Robadob deleted the callback branch December 7, 2022 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants