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

Fixing memory leak in Surface #655

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fixing memory leak in Surface #655

wants to merge 2 commits into from

Conversation

LordEidi
Copy link
Contributor

This is an ugly hack but fixes the memory leak in Surface for now. A refactoring would be the better option.

@Daft-Freak
Copy link
Collaborator

Hmm, another one in load_from_bmp:

if(!data)
data = new uint8_t[header.image_size];
auto ret = new Surface(data, format, bounds);

Also, tooManyCamels, not_enough_snakes 😄

@Gadgetoid
Copy link
Contributor

Quite amazed this hasn't been conflicted yet. I guess the engine doesn't get quite the TLC the ports do!

Seems the reason this failed for Emscripten has been lost to the sands of time. Could use a rebase and some gentle nudging into snake_case as suggested.

@LordEidi got any time/interest in doing this, or should we grab and tidy up?

@Daft-Freak
Copy link
Collaborator

Until recently (in uh, "SDK time") changing the layout of Surface would break the firmware API. That's possibly why nothing much has changed here.

As an additional comment, the new variables should probably be up with the other variables.

(This was on my list of things to look at, which I should probably be less secretive about...)

@LordEidi
Copy link
Contributor Author

Sorry, wasn't on Github lately (Codeberg is a nice place to be :) ).

Re your question regarding tidying up. We moved away from loading our data through blit::Surface and implemented an Aseprite exporter which generates code (two arrays: one image, one palette and a struct for the rest) which is then directly compiled into the binary. We feed that data structure into the Surface during execution and haven't had any trouble with that so far.

Long story short: I could help you brushing up this pull request. But I haven't followed the 32blit SDK changes lately as closely as I used to. I would need a bit of guidance what you need to accept the change.

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.

3 participants