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

Fixed bugs with Line_PortalSetTarget and added more portal geometry warnings #373

Merged
merged 2 commits into from Oct 3, 2017

Conversation

AntiBlueQuirk
Copy link
Contributor

  • A bug exists where portals that have been deactivated with Line_PortalSetTarget cannot be reactivated, even if given a valid target.
  • Another bug exists where portals that were created in an inactive state (using a target line tag of 0) could never be activated. (Even with the above bugfix.)
  • Linked portals that have been demoted to teleport portals because they do not have a return portal now emit a warning.
  • Portals that are supposed to be traversable, but do not have a back-sector now demote to visual portals and emit a warning, because nothing could ever possibly traverse them anyway.

I see now that there's another pull request (#372) to fix the first bug in a slightly different way. I'm not sure which change is more appropriate, since I'm barely familiar with the portal code, but this is my take on it. It looks like the mFlags member is the main method of tracking whether portals are active, so it looks like it should always updated in the ChangePortalLine function.

I updated Gez's original portal test map from the original portal thread. I got everything working except for the switchable portals, which is what caused me to stumble on these bugs. Here's the updated version for testing: zdoomx_test0_upd.zip

…arnings

- A bug exists where portals that have been deactivated with Line_PortalSetTarget cannot be reactivated, even if given a valid target.
- Another bug exists where portals that were created in an inactive state (using a target line tag of 0) could never be activated. (Even with the above bugfix.)
- Linked portals that have been demoted to teleport portals because they do not have a return portal now emit a warning.
- Portals that are supposed to be traversable, but do not have back-sector now demote to visual portals and emit a warning, because nothing could ever possibly traverse them anyway.
@gwHero
Copy link
Contributor

gwHero commented Sep 30, 2017

Interesting :) My pull request attacks indeed the same issue. My thoughts about your second point (portals that were created in an inactive state) were simply that I concluded that they had to exist during map start with a destination. Well, it's good to see somebody else sharing improvements regarding this. Hopefully at least one solution will be addressed :)

@AntiBlueQuirk
Copy link
Contributor Author

The code clearly supports portals with no destination, which is why you can do Line_SetPortalTarget(id, 0) to disable them. Interestingly, if the portal has no destination during creation, everything is set up properly,
except that it intentionally sets the mDefFlags field to 0. This means that even when that portal gets a valid destination later, it won't work right, since it doesn't have any features enabled.

I removed the check so that all portals get their mDefFlags field set up properly. Later code (in P_UpdatePortal) still makes sure they're disabled properly by clearing mFlags, so I'm not sure why mDefFlags was ever cleared like that.

You can see this feature in the test wad I uploaded. The number portals (except for the first) all start in a disabled state with Line_SetPortal(0, 0, 2, 0). All the normal portal initialization happens, except that they don't have a target, so they get disabled. An ACS script later updates their targets as needed, enabling them.

@gwHero
Copy link
Contributor

gwHero commented Sep 30, 2017

Your approach goes one step further. I only addressed the fact that after putting a portal in a disabled state, it could not be reenabled again. If your code is approved, it would be even more convenient. But we will have to wait for Graf's opinion.

Interactive portals demoted to visual due to not having a back-sector would not have their interactive flag properly cleared.
@coelckers coelckers merged commit 9e07bab into ZDoom:master Oct 3, 2017
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