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

call pendingDestroy when add image #117

Merged
merged 3 commits into from
Aug 27, 2019
Merged

Conversation

taoabc
Copy link
Contributor

@taoabc taoabc commented Aug 16, 2019

Call json.addToCache will call pendingDestroy internally, but add image to textureManager will not, so I call pendingDestroy manually.

@akdcl
Copy link
Member

akdcl commented Aug 17, 2019

@jcyuan

@jcyuan
Copy link
Contributor

jcyuan commented Aug 21, 2019

👍 good catch, a potential bug of memory leak.
but we can just use image.addToCache(); instead.
the old code should be a legacy problem made when immigrating to the new db file plugin.

Copy link
Contributor

@jcyuan jcyuan left a comment

Choose a reason for hiding this comment

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

could please change to image.addToCache()

@taoabc
Copy link
Contributor Author

taoabc commented Aug 22, 2019

I don't recommend add this image to cache because we already add image to textureManager, will become more heavier. What do you think? 😁

@jcyuan
Copy link
Contributor

jcyuan commented Aug 23, 2019

image.addToCache, this action actually does 2 things:
1, add the loaded image into the textureManager belongs to the loader. (global to scene)
2, call pendingDestroy to prepare to delete the stuffs created when loading the images

only a diffenent between them is, addToCache detects to see if the image to be loaded has a normal map or not, if it does, add to cache too.

so actually they are the same, but use Phaser's API is more sensible because you don't knwo whether they will be changed in the furture or not, right?

@taoabc
Copy link
Contributor Author

taoabc commented Aug 25, 2019

You're right, I've changed to addToCache

Copy link
Contributor

@jcyuan jcyuan left a comment

Choose a reason for hiding this comment

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

you forgot to remove line this.loader.textureManager.addImage(image.key, image.data); , as it becomes useless now. 🍭

@taoabc
Copy link
Contributor Author

taoabc commented Aug 26, 2019

Sorry for misunderstand your suggest, fixed and refresh output files at newest commit.

@jcyuan jcyuan merged commit 4f7a64a into DragonBones:master Aug 27, 2019
@jcyuan
Copy link
Contributor

jcyuan commented Aug 27, 2019

thanks for your effort @taoabc 👍

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

3 participants