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

Allow ReEnable portal with Line_SetPortalTarget also for Visual&Teleport #372

Closed
wants to merge 1 commit into from
Closed

Conversation

gwHero
Copy link
Contributor

@gwHero gwHero commented Sep 25, 2017

Changing a portal destination at runtime with Line_SetPortalTarget after it was temporarily disabled with destination 0 (zero) only worked for PORTF_INTERACTIVE and not for PORTT_VISUAL & PORTT_TELEPORT, while the WIKI states that it's only not allowed for static portals (type 3 & 4, see https://zdoom.org/wiki/Line_SetPortal).
Actually the only thing that was missing was restoring the mFlags for the Visual & Teleport types.

I have tested this change with a build on my system with a test wad which I will upload to the ZDoom forums. Since this is my first contribution the GZDoom code ever I will humble await the verdict of the more experienced programmers :)
PortalMAP01.zip

I have included a small testwad that disables a portal after map startup; the switch in the foreground will bring it back, and with the switch in the background it can be hidden again. This did not work with the original code for Visual & Teleport types.

…leport

Changing a portal destination at runtime with Line_SetPortalTarget after it was temporarily disabled with destination 0 (zero) only worked for PORTF_INTERACTIVE and not for PORTT_VISUAL & PORTT_TELEPORT, while the WIKI states that it's only not allowed for static portals (type 3 & 4, see https://zdoom.org/wiki/Line_SetPortal).
Actually the only thing that was missing was restoring the mFlags for the Visual & Teleport types.
@alexey-lysiuk
Copy link
Collaborator

Why did you changed so many lines? You can just add the following at line 469:

else if (port->mType == PORTT_VISUAL || port->mType == PORTT_TELEPORT)
{
	port->mFlags = port->mDefFlags;
}

@gwHero
Copy link
Contributor Author

gwHero commented Sep 25, 2017

Thanks for the fast reply! Actually you're right; I also added brackets to the existing "else if" and the one I added - it's more of a practice I am used to in my own projects to enhance the readability, but indeed some more code.

@gwHero
Copy link
Contributor Author

gwHero commented Sep 25, 2017

I see now indeed that Github says that I changed 26 lines but that's a bit exaggerated; it's because of the inserted brackets. Nevertheless: if you prefer without the brackets, do you want me to redo the pull request without them?

@alexey-lysiuk
Copy link
Collaborator

Yes, I prefer to reduce amount of changes to minimum. Tracking who changed something and why is easier when no pointless modifications were introduced.
Of course it’s up to Graf to disagree with this and merge it as is. Although existing style is easier to read to me.
Actually you need to wait for his opinion anyway as I’m not quite familiar with portals.

@gwHero
Copy link
Contributor Author

gwHero commented Sep 25, 2017

Ah ok, I'll wait to see what Graf thinks about the change in relation to the portal functionality. Maybe he will reject it anyway. If not, and he agrees with the change, it's of course fine with me to remove the brackets which I added when I was debugging the code to see what the problem was with the Line_SetPortalTarget - my personal preference is of course far less important than the tracking of changes - which is a very good point.

Thank you for the feedback!

@coelckers
Copy link
Member

I have to agree with alexey-lysiuk on the formatting issue.. The more you change the harder it is to review a change. And the indentation here is totally pointless.

@alexey-lysiuk
Copy link
Collaborator

As #373 fixes the same issue without altering code formatting I’m closing this PR.

@gwHero
Copy link
Contributor Author

gwHero commented Oct 1, 2017

Okay, I'll pay intention next time not to change indentations/brackets and such to make the actual change in functionality more readible. Hopefully #373 will get passed, because the wad I'm working on needs this 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.

None yet

3 participants