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

Move questinfo data from map to npc_data #2433

Merged
merged 3 commits into from Jun 1, 2019

Conversation

Asheraf
Copy link
Contributor

@Asheraf Asheraf commented Apr 10, 2019

Pull Request Prelude

Issues addressed

#2431

Changes Proposed

fix memory leak in setquestinfo

@HerculesWSAPI
Copy link
Contributor

This change is Reviewable

@Asheraf
Copy link
Contributor Author

Asheraf commented Apr 10, 2019

@AnnieRuru can you check if this fixes your problem.

@AnnieRuru
Copy link
Contributor

no, still the same

Memory manager: Memory leaks found at 2019/04/11 10h36m10s (Git rev 'ab81d4012eac5c2c00c485971fc9b89bf69761be').
0001 : d:\ragnarok\hercules\src\map\map.c line 5986 size 744 address 0x043A1604
0002 : d:\ragnarok\hercules\src\map\script.c line 21344 size 8 address 0x06D0015C
0003 : d:\ragnarok\hercules\src\map\script.c line 21300 size 12 address 0x06D0453C

note the line number is different than in the issue, as I have apply your patch

also you can test it yourself by type atcommand @mapexit
and open log/map-server.exe.leaks file

prontera,155,180,5	script	sdfasdf	1_F_MARIA,{
	end;
OnInit:
	questinfo QTYPE_QUEST, 1;
	setquestinfo QINFO_QUEST, 1001, 1;
	questinfo QTYPE_QUEST, 1;
	setquestinfo QINFO_ITEM, Apple, 1, 30000;
	end;
}

@Asheraf Asheraf force-pushed the questinfomemfix branch 2 times, most recently from c501b6f to 74dd125 Compare April 12, 2019 11:16
@Asheraf
Copy link
Contributor Author

Asheraf commented Apr 12, 2019

@AnnieRuru this should fix the memory leak for now, but seems like I found another "bug" i will try to do an update later, hopefully before next release date.

@AnnieRuru
Copy link
Contributor

AnnieRuru commented Apr 12, 2019

... urgh ... okay ... because this still doesn't fix the memory leak yet though ...

Memory manager: Memory leaks found at 2019/04/12 20h24m31s (Git rev 'ab81d4012eac5c2c00c485971fc9b89bf69761be').
0001 : d:\ragnarok\hercules\src\map\map.c line 6036 size 744 address 0x04EC1624
0002 : d:\ragnarok\hercules\src\map\script.c line 21344 size 8 address 0x0890C53C
0003 : d:\ragnarok\hercules\src\map\script.c line 21300 size 12 address 0x0890C55C

but at least I see the code clean up, and know where the responsible code for it

EDIT: yeah stupid me should've notice the line number is same as in the issue, means I didn't compile

@Asheraf
Copy link
Contributor Author

Asheraf commented Apr 12, 2019

@AnnieRuru I am following the steps you gave to reproduce this and not getting any leaks.
however i see one place left when reloading single npc.

[Status]: Finished.
[Info]: Memory manager: No memory leaks found.                                                                                                                           ```

Copy link
Contributor

@AnnieRuru AnnieRuru left a comment

Choose a reason for hiding this comment

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

Ooopsie ~ yeah ... I forgot to compile hahaha

tested with some of my other custom quest log script, didn't see any leak anymore

@AnnieRuru AnnieRuru added this to the Release v2019.05.05 milestone Apr 12, 2019
@Asheraf Asheraf added the status:on-hold The issue was put on hold until another issue is solved or a feature is implemented label Apr 13, 2019
@Asheraf Asheraf changed the title fix memory leak in setquestinfo Move questinfo data from map to npc_data Apr 29, 2019
@Asheraf Asheraf removed the status:on-hold The issue was put on hold until another issue is solved or a feature is implemented label Apr 29, 2019
@Asheraf
Copy link
Contributor Author

Asheraf commented Apr 29, 2019

@AnnieRuru I have moved this around to fix a bug i missed last time, memory leaks should be fixed too, let me know if you find other problems.

@Asheraf Asheraf added the status:code-review Awaiting code review label May 4, 2019
this will fix the issue where having multiple `questinfo()` blocks wont work properly
@MishimaHaruna MishimaHaruna merged commit c8202e1 into HerculesWS:master Jun 1, 2019
@Asheraf Asheraf deleted the questinfomemfix branch June 2, 2022 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:code-review Awaiting code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants