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

Door protection fix, and fallback compatibility code for mods. #858

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
8 participants
@sofar
Copy link
Member

commented Feb 20, 2016

During a previous fix, I overwrote def causing door protection to be broken. This affected newly placed doors only, so it was a bit tricky to spot.

A second patch includes an almost entirely functional replacement for the deprecated door.register_door() API, that will replace old style doors for any mod to new style doors.

The only thing it can't replace is the tiles field. It prints out a solid warning about that, and hopefully helps mod developers to convert quickly. If the tiles door def is found, no warning is displayed and everything should work well.

I've tested the door API replacement with an old version of "mydoors" (https://github.com/DonBatman/mydoors) which has a ton of doors.

def.only_placer_can_open = nil
local i = name:find(":")
local modname = name:sub(1, i - 1)
local doorname = modname .. "_" .. name:sub(i + 1, -1)

This comment has been minimized.

Copy link
@kilbith

kilbith Feb 20, 2016

Contributor

Please take the habit of separating a list of variables from the rest with blank newlines. One variable is fine, above that -> you isolate them.

@kilbith

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2016

Also choose a better PR name.

@proller

This comment has been minimized.

Copy link

commented Feb 20, 2016

also after upgrade my server spams with
2016-02-20 15:08:13: ERROR[Server]: Item "doors:door_steel_b_1" not defined
2016-02-20 15:08:13: ERROR[Server]: Item "doors:door_steel_t_2" not defined

@sofar

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2016

@proller with, or without this patch?

@sofar sofar changed the title Door fixes(2) Door protection fix, and fallback compatibility code for mods. Feb 20, 2016

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2016

Fixed the door issue for me, now I can remove steel doors placed by me.

@sofar

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2016

Just a few cosmetic updates: spacing, typo, comment.

@proller

This comment has been minimized.

Copy link

commented Feb 20, 2016

without, i'm waiting for merge to test

@sofar

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2016

@proller those nodes are getting replaced by an ABM if you're on a current minetest_game version. Those messages may pop up on map load, but the doors should be converted properly. If that is not the case -> file a separate issue plz.

@tenplus1

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2016

+1 on backwards compatibility for mods :)

@sofar

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2016

@paramat, thanks, updated.

@sofar

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2016

@paramat since testing these door patches separately is a bit of a kludge, I've included the patched version of #861 in here as third patch, since that seems appropriate.

I can now safely use the unmodified mydoors with minetest_game from 9e54b37 and after this series without modifying it.

@sofar

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2016

Sadly, this doesn't fix the landrush mod entirely, since that mod overrides the on_rightclick handlers of the subdoor types. However, I believe that's a fairly straightforward and simple change that the mod owner can make.

it has made me very aware that our node protection API is terrible

@paramat

This comment has been minimized.

Copy link
Member

commented Feb 21, 2016

Also in the 3rd commit i can see more spaces inside curly brackets in the surrounding code, if it's possible remove them, or if not leave it until a future commit.

@sofar

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2016

Repushed with the whitespace changes.

@paramat

This comment has been minimized.

Copy link
Member

commented Feb 21, 2016

👍

@oleastre

This comment has been minimized.

Copy link

commented Feb 21, 2016

Looks good to me to solve #861 and #854.
Thanks for your support.

@paramat

This comment has been minimized.

Copy link
Member

commented Feb 21, 2016

Approval from sfan5 http://irc.minetest.ru/minetest-dev/2016-02-21#i_4540303
Will merge soon.

@oleastre

This comment has been minimized.

Copy link

commented Feb 21, 2016

Small remark, with the latest changes, aren't the two ABM the same ? In doors.register and doors.register_door ?
I'm just checking from my phone so can not do a real comparison.

@paramat

This comment has been minimized.

Copy link
Member

commented Feb 21, 2016

Yes but the 2nd ABM is for mods using the old 'register door' code. So it seems correct, and sofar tested it.

@oleastre

This comment has been minimized.

Copy link

commented Feb 21, 2016

For me, it seems that since register_doors calls doors.register, the same ABM is registered two times.
So yes, it works but if I'm not mistaken, it is redundant.

@paramat

This comment has been minimized.

Copy link
Member

commented Feb 21, 2016

Ah, good point, i'll wait for sofar then.

Fix broken door protection.
A previous fix overwrote the `def` variable during registration,
causing protected doors no longer to be placed with protection.
@sofar

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2016

Well spotted, it is indeed obsolete now.

Removed it, re-tested the pre-meshdooors->patch transition and all OK

else
def.tiles = {{name = "doors_door_wood.png", backface_culling = true}}
end
minetest.log("error", modname .. " registered door \"" .. name .. "\" using deprecated API method \"doors.register_door()\" but did not provide the \"tiles\" parameter. A fallback tiledef will be used instead.")

This comment has been minimized.

Copy link
@kaeza

kaeza Feb 21, 2016

Contributor

Can you break this line a bit?
Also, it's technically a warning and not an error.

sofar added some commits Feb 20, 2016

Fallback doors.register_door() code.
This function maps doors.register_door to the new API as far as
reasonable. We can't map the texture, so we fall back to a default
texture. An error message is printed if mod writers did not provide the
needed new tiles field for the door. The created doors are functional
and a full replacement. Old doors are replaced with the new ones
through an ABM.
Allow mod namespace for door registrations.
This is an adapted version of #861 - by oleastre

Most mods had been calling `doors.register_door() with a door
name that included the "modname:" prefix, and we should continue
to allow mods to do so, without registering the nodenames created
in the "doors:" namespace.

The default case is to use the "modname:" prefix verbatim. If
mods or code calls this function without a prefix, then "doors:"
is automatically used.

Now that the namespace is corrected, the copy replacement ABM is
no longer needed.
@sofar

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2016

@kaeza updated - broke up the line and made it a warning.

@kaeza

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2016

👍

@paramat

This comment has been minimized.

Copy link
Member

commented Feb 22, 2016

👍 Will merge soon then.

@paramat

This comment has been minimized.

Copy link
Member

commented Feb 23, 2016

@paramat paramat closed this Feb 23, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.