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

Export FBoundingBox to ZScript. #658

Closed
wants to merge 1 commit into from

Conversation

Doom2fan
Copy link
Contributor

@Doom2fan Doom2fan commented Dec 7, 2018

Copy link
Member

@coelckers coelckers left a comment

Choose a reason for hiding this comment

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

Looking at this, I see two immediate issues:

  1. Having access functions to single variables because the internal fields are badly named is not good. Most of the exported methods are mere setters and getters.
    Better export it in a way that the fields can be used directly, especially for a pure data class like this one.

  2. If you export a data class as an object it needs to be serializable.

@Doom2fan
Copy link
Contributor Author

Doom2fan commented Dec 26, 2018

All the getters and setters are the same as FBoundingBox's. m_Box cannot be exported (At least as far as I know), as it's a protected variable, not public.
If you want me to, however, I can merge the individual getters into a single "Get" function.

That said, I'd like to know if there's any way to export FBoundingBox itself - I had to create a new class named "DBoundingBox", as FBoundingBox doesn't inherit from DObject, and making it inherit from DObject causes problems, as GZDoom's code frees it manually currently.

@Doom2fan
Copy link
Contributor Author

Doom2fan commented Dec 26, 2018

I can't seem to get it to serialize correctly - whenever I load a save m_Box's values are wrong, so I'm honestly just gonna give up on this.

This all feels like the wrong way to do this - the actual FBoundingBox class should be exported, and all the places where GZDoom allocates/creates or frees it manually should be changed - That way it'd have more uses instead of pretty much being only being useful for mods.

@Doom2fan Doom2fan closed this Dec 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants