Add NoAutoloot mapflag #1833

Emistry · 2017-08-27T17:28:02Z

Pull Request Prelude

I have followed proper Hercules code styling.
I have read and understood the contribution guidelines before making this PR.
I am aware that this PR will be closed if the above-mentioned criteria are not fulfilled.

Changes Proposed

Autoloot are useful and popular among the players.
However, sometimes when we try to restrict the autoloot usage at certain map, we can only rely on mapflag nocommand to disable the autoloot, or some tricky ways of using the npc script (bindatcmd/OnPCLoadMapEvent..etc).
With this mapflag, server has more control over the autoloot usage and prevent player abuse the autoloot feature when the server doesn't want them to use it at certain maps.

Affected Branches:

: Master

Issues addressed:
Enable server to disable autoloot settings for certain maps.

HerculesWSAPI · 2017-08-27T17:28:09Z

This change is

phaicm · 2017-08-27T22:53:31Z

src/map/atcommand.c

 	if (map->list[m_id].flag.guildlock)
 		strcat(atcmd_output, msg_fd(fd,1097)); // GuildLock |
+	if (map->list[m_id].flag.noautoloot)
+		strcat(atcmd_output, "No Autoloot |"); // NoViewID |


Change comment appropriately

lol, will change it

dastgirp · 2017-08-28T07:06:08Z

src/map/atcommand.c

 	int i;
 	int action = 3; // 1=add, 2=remove, 3=help+list (default), 4=reset

+					// Disable autoloot


dastgirp · 2017-08-28T07:07:43Z

src/map/mob.c


 	if( sd
-		&& (drop_rate <= sd->state.autoloot || pc->isautolooting(sd, ditem->item_data.nameid))
+		&& ((drop_rate <= sd->state.autoloot || pc->isautolooting(sd, ditem->item_data.nameid)) && !map->list[sd->bl.m].flag.noautoloot)


Does not need this, instead you can edit
pc_isautolooting to return false if player is on noautoloot

but i think the pc_isautolooting doesnt cover the sd->state.autoloot

dastgirp · 2017-08-28T07:08:21Z

src/map/pc.c

 		pc->setrestartvalue(sd,1);
 	}

+	if (map->list[m].flag.noautoloot) { // Disable autoloot


Does not need this, as this would be doing unnecessary setting of variables to 0, even if they are 0.
pc_isautolooting should be enough

Also, this would mean.
Warping from noautoloot map to other maps where autoloot is allowed would make autoloot list empty.

removed this part, auto loot list will be remain upon enter/leave the map with noautoloot mapflag.

dastgirp · 2017-09-04T02:07:16Z

Should we do the way it is now(not allowing to turn autoloot on/off or add items) or allow autoloot commands but also display message that autoloot is off in current map?

dastgirp · 2017-09-04T02:08:32Z

Also just a message when warping to new map would be good. To let player know autoloot is off.

Emistry · 2017-09-04T15:22:24Z

Should we do the way it is now(not allowing to turn autoloot on/off or add items) or allow autoloot commands but also display message that autoloot is off in current map?

I think its good to turn it off. Prevent player asking "Why I can use @autoloot but its not looting any items."

Also just a message when warping to new map would be good. To let player know autoloot is off.

erm, I think should be fine without any message to notify them.... just like any other mapflags (nosave, nocommand) we doesn't display any message to alert them that these commands are disabled when they enter the map.

Helianthella

tested, works fine

MishimaHaruna · 2017-10-16T14:15:11Z

The way this is implemented doesn't offer a great user experience in my opinion. Some critical points that I'd like to be addressed before merging this:

If an user has autoloot enabled, and wants to disable it while in a map with this flag, they won't be able to. They instead see a message saying that auto looting items is disabled in the current map (but that's not what they were trying to do to begin with).
Assuming that a server implements an OnPCLoginEvent that enables autoloot for the users, it'll fail if they log in while in a map with this flag (and won't turn on if they later leave that map).

I'd personally prefer to have the ability to toggle the setting regardless of whether the map allows autolooting. Since autoloot is a global setting attached to the player (rather than attached to the map), then maps shouldn't disallow any changes to the setting, but just disable its effect.

Emistry · 2017-10-18T16:59:33Z

Removed the restriction to use the atcommands in the map that has this mapflag.

MishimaHaruna

Ok, just one more thing: db/constants.conf needs a mf_noautoloot entry (and an update to mv_noviewid, since its value changed. I believe it's ready to merge afterwards

- Enable server to disable autoloot settings for certain maps.

Emistry · 2017-10-21T16:16:04Z

Aw, didn't realize that. Thank you and I have updated it.

MishimaHaruna · 2017-10-21T17:31:57Z

Thank you, I'm merging it now

Emistry added status:inprogress Issue is being worked on / the pull request is still a WIP type:enhancement Issue describes an enhancement or feature that should be implemented labels Aug 27, 2017

Emistry self-assigned this Aug 27, 2017

Emistry requested review from MishimaHaruna and dastgirp August 27, 2017 17:28

Emistry force-pushed the mapflag_noautoloot branch from bb31943 to 22e3fe9 Compare August 27, 2017 18:11

phaicm reviewed Aug 27, 2017

View reviewed changes

dastgirp requested changes Aug 28, 2017

View reviewed changes

Emistry force-pushed the mapflag_noautoloot branch 2 times, most recently from 1ccde8e to 94f81f8 Compare September 3, 2017 07:29

Emistry removed the status:inprogress Issue is being worked on / the pull request is still a WIP label Sep 3, 2017

Helianthella added this to the next release milestone Oct 8, 2017

Helianthella approved these changes Oct 10, 2017

View reviewed changes

MishimaHaruna added the codereview:needsedits Some edits have been requested before the pull request can be accepted label Oct 16, 2017

Emistry force-pushed the mapflag_noautoloot branch from 94f81f8 to 4aac1d7 Compare October 18, 2017 16:57

ghost added the status:inprogress Issue is being worked on / the pull request is still a WIP label Oct 18, 2017

Emistry removed the status:inprogress Issue is being worked on / the pull request is still a WIP label Oct 18, 2017

MishimaHaruna requested changes Oct 21, 2017

View reviewed changes

Add NoAutoloot mapflag

2c2839c

- Enable server to disable autoloot settings for certain maps.

Emistry force-pushed the mapflag_noautoloot branch from 4aac1d7 to 2c2839c Compare October 21, 2017 16:15

ghost added the status:inprogress Issue is being worked on / the pull request is still a WIP label Oct 21, 2017

MishimaHaruna approved these changes Oct 21, 2017

View reviewed changes

MishimaHaruna removed the codereview:needsedits Some edits have been requested before the pull request can be accepted label Oct 21, 2017

MishimaHaruna removed the status:inprogress Issue is being worked on / the pull request is still a WIP label Oct 21, 2017

Helianthella approved these changes Oct 21, 2017

View reviewed changes

dastgirp approved these changes Oct 21, 2017

View reviewed changes

MishimaHaruna merged commit 58fdbea into HerculesWS:master Oct 21, 2017

Emistry deleted the mapflag_noautoloot branch October 21, 2017 18:01

Add NoAutoloot mapflag #1833

Add NoAutoloot mapflag #1833

Uh oh!

Conversation

Emistry commented Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Prelude

Changes Proposed

Uh oh!

HerculesWSAPI commented Aug 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dastgirp commented Sep 4, 2017

Uh oh!

dastgirp commented Sep 4, 2017

Uh oh!

Emistry commented Sep 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Helianthella left a comment

Choose a reason for hiding this comment

Uh oh!

MishimaHaruna commented Oct 16, 2017

Uh oh!

Emistry commented Oct 18, 2017

Uh oh!

MishimaHaruna left a comment

Choose a reason for hiding this comment

Uh oh!

Emistry commented Oct 21, 2017

Uh oh!

MishimaHaruna commented Oct 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Emistry commented Aug 27, 2017 •

edited

Loading

Emistry commented Sep 4, 2017 •

edited

Loading