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

Export F3DFloor structure and related data #732

Merged
merged 1 commit into from Sep 11, 2019

Conversation

@OrdinaryMagician
Copy link
Contributor

OrdinaryMagician commented Feb 4, 2019

Cleaner version of former PR. All the exported data is read-only from the zscript side, due to its nature.

Additional changes:

  • 3D floor related parts of extsector_t are also exported.
  • The ffloor member on LineTracer results is now uncommented.
  • After hitting a floor/ceiling, TraceTraverse will now assign ffloor to the appropriate 3D floor that was hit, if any.

Notes:

  • Only exposed what's useful for modders.
  • PlaneRef not directly exported unlike in previous PR, as most of the data it contains is redundant after 3D floor setup. Instead, bottom and top in ZScript F3DFloor are set to their secplanes.
OrdinaryMagician added a commit to OrdinaryMagician/flak_m that referenced this pull request Feb 8, 2019
@OrdinaryMagician

This comment has been minimized.

Copy link
Contributor Author

OrdinaryMagician commented Feb 20, 2019

Well... now how would I deal with a conflict after a file is moved in master?

@caligari87

This comment has been minimized.

Copy link
Contributor

caligari87 commented Feb 20, 2019

Does a rebase help at all? If not you might have to manually re-apply your changes. There's some other solutions here like editing the diff to point to the moved file.

@OrdinaryMagician OrdinaryMagician force-pushed the OrdinaryMagician:f3dfloor_export branch from a6a285f to 15e88bc Feb 20, 2019
@OrdinaryMagician

This comment has been minimized.

Copy link
Contributor Author

OrdinaryMagician commented Feb 20, 2019

OK that clearly didn't work even though I followed the instructions clearly. I guess I have to re-do the PR

@OrdinaryMagician

This comment has been minimized.

Copy link
Contributor Author

OrdinaryMagician commented Feb 20, 2019

Wait I wasn't pulling from upstream nvm I can fix this.

@OrdinaryMagician OrdinaryMagician force-pushed the OrdinaryMagician:f3dfloor_export branch from 15e88bc to fe61170 Feb 20, 2019
@OrdinaryMagician

This comment has been minimized.

Copy link
Contributor Author

OrdinaryMagician commented Feb 20, 2019

Someone please call Graf before this gets worse.

@OrdinaryMagician OrdinaryMagician force-pushed the OrdinaryMagician:f3dfloor_export branch from fe61170 to a909473 Feb 23, 2019
@OrdinaryMagician

This comment has been minimized.

Copy link
Contributor Author

OrdinaryMagician commented Feb 23, 2019

I'm going to keep this alive no matter how many times files are moved around.

@MajorCooke

This comment has been minimized.

Copy link
Contributor

MajorCooke commented Feb 23, 2019

Try PMing him and ask him why he's waiting.

Though to that end, didn't he say he was waiting for vulkan or something?

@OrdinaryMagician

This comment has been minimized.

Copy link
Contributor Author

OrdinaryMagician commented Apr 21, 2019

I'm definitely going to keep this alive no matter what. I still have hope.

Copy link
Collaborator

alexey-lysiuk left a comment

It looks OK to me except extsector e member. Could you please give it a meaningful name?

@OrdinaryMagician

This comment has been minimized.

Copy link
Contributor Author

OrdinaryMagician commented Jul 6, 2019

I would if I knew what its more descriptive name was supposed to be, because it doesn't have one.

Alternatively I could simply get rid of that altogether and make its data readable from the sector struct itself... except previous attempts at doing that just resulted in crashing / garbage data.

@alexey-lysiuk

This comment has been minimized.

Copy link
Collaborator

alexey-lysiuk commented Jul 6, 2019

extended or something like that would be better than just e.

Copy link
Owner

coelckers left a comment

extsector_t should remain private to the engine. Please replace with getter functions. If this gets directly exposed it will forever block refactoring.

Also, getter functions for the plane textures should be added, not having these makes no sense.

@OrdinaryMagician

This comment has been minimized.

Copy link
Contributor Author

OrdinaryMagician commented Jul 7, 2019

extsector_t should remain private to the engine. Please replace with getter functions. If this gets directly exposed it will forever block refactoring.

Will do.

Also, getter functions for the plane textures should be added, not having these makes no sense.

This sounds redundant since they can be obtained from the F3DFloor's model sector (which is accessible), but okay.

OrdinaryMagician added a commit to OrdinaryMagician/flak_m that referenced this pull request Jul 7, 2019
@coelckers

This comment has been minimized.

Copy link
Owner

coelckers commented Jul 7, 2019

This sounds redundant since they can be obtained from the F3DFloor's model sector (which is accessible), but okay.

So are the planes.
But don't forget that the relationship can be swapped. Vavoom-style definitions have floor->floor and ceiling->ceiling, while regular 3D floors have floor->ceiling and ceiling->floor relations. That shouldn't be something the scripter has to be aware of.

@OrdinaryMagician

This comment has been minimized.

Copy link
Contributor Author

OrdinaryMagician commented Jul 7, 2019

Oh oops, I'll have to change that again then.

Copy link
Collaborator

alexey-lysiuk left a comment

You need to check indices for validity before accessing array elements. I would do this in native functions, and call them from VM counterparts to avoid copy-pasting.

@OrdinaryMagician

This comment has been minimized.

Copy link
Contributor Author

OrdinaryMagician commented Jul 7, 2019

Would that sort of overhead be needed when there's already a function to get the size of the array?

@alexey-lysiuk

This comment has been minimized.

Copy link
Collaborator

alexey-lysiuk commented Jul 7, 2019

Yes, there must be no way to crash the engine from a script. Invalid indices should be either ignored, or VM abort exception should be raised. Performance wise it’s almost nothing in comparison with VM function call.

@OrdinaryMagician

This comment has been minimized.

Copy link
Contributor Author

OrdinaryMagician commented Jul 7, 2019

Since you mention that, I should also update Shape2D in another PR to prevent out of bounds access too. Gutawer recently mentioned to me that can happen.

@madame-rachelle

This comment has been minimized.

Copy link
Collaborator

madame-rachelle commented Jul 12, 2019

Since you mention that, I should also update Shape2D in another PR to prevent out of bounds access too. Gutawer recently mentioned to me that can happen.

Please do that - and also please always include bounds checks in future PR's. C and C++ are both the most popular languages still in use today that are vulnerable to that, and it is always beholden on the programmer to do such checks. To not to it is, to say the very least, extremely irresponsible.

@OrdinaryMagician

This comment has been minimized.

Copy link
Contributor Author

OrdinaryMagician commented Jul 12, 2019

I will happily accept this scolding. That "veteran C programmer" part of myself needs to go away.

@@ -212,6 +212,55 @@ struct SecPlane native play
native double PointToDist(Vector2 xy, double z) const;
}

struct F3DFloor native play
{
enum EFFloorType

This comment has been minimized.

Copy link
@alexey-lysiuk

alexey-lysiuk Jul 12, 2019

Collaborator

I would change it to something like F3DFloorFlags as it doesn't represent a floor type but combination of its properties. This type name shouldn't be used directly though.

This comment has been minimized.

Copy link
@OrdinaryMagician

OrdinaryMagician Jul 12, 2019

Author Contributor

Done

@madame-rachelle

This comment has been minimized.

Copy link
Collaborator

madame-rachelle commented Jul 28, 2019

This branch now has conflicts which will interfere with it being merged.

@OrdinaryMagician

This comment has been minimized.

Copy link
Contributor Author

OrdinaryMagician commented Jul 28, 2019

Blame it on whoever decided to move p_trace.cpp again

I forgot how to rebase.

commit 6ecd831
Merge: a4fb1f6 afbd7f7
Author: Marisa Kirisame <marisa@sayachan.org>
Date:   Sun Jul 28 22:02:19 2019 +0200

    Merge branch 'master' of github.com:coelckers/gzdoom into f3dfloor_export

commit a4fb1f6
Author: Marisa Kirisame <marisa@sayachan.org>
Date:   Fri Jul 12 15:43:25 2019 +0200

    Renamed EFFloorType enum in ZScript to the more descriptive EF3DFloorFlags

commit 9ad1c3b
Author: Marisa Kirisame <marisa@sayachan.org>
Date:   Sun Jul 7 20:25:31 2019 +0200

    Add bounds checks to Get3DFloor/GetAttached

commit dd2a795
Author: Marisa Kirisame <marisa@sayachan.org>
Date:   Sun Jul 7 17:13:44 2019 +0200

    Correct handling of 3d floor plane texture getting.

commit 9b74828
Author: Marisa Kirisame <marisa@sayachan.org>
Date:   Sun Jul 7 16:14:45 2019 +0200

    Implemented requested changes to F3DFloor exports.
    * Getters for ffloors/attached arrays.
    * Getter for 3D floor top/bottom texture.

commit 6a1482b
Author: Marisa Kirisame <marisa@sayachan.org>
Date:   Sat Jul 6 13:42:52 2019 +0200

    Renamed exported extsector pointer in Sector struct to something more descriptive.

commit 7c6783d
Merge: ff64e04 8d36f0a
Author: Marisa Kirisame <marisa@sayachan.org>
Date:   Mon Apr 29 12:40:44 2019 +0200

    Merge branch 'master' into f3dfloor_export

commit ff64e04
Merge: a909473 5b6bae4
Author: Marisa Kirisame <marisa@sayachan.org>
Date:   Sun Apr 21 16:56:18 2019 +0200

    Merge branch 'master' into f3dfloor_export

commit a909473
Author: Marisa Kirisame <marisa@sayachan.org>
Date:   Mon Feb 4 17:47:25 2019 +0100

    Export F3DFloor structure and related data.
    Small changes to Trace code to better use this struct.
@OrdinaryMagician OrdinaryMagician force-pushed the OrdinaryMagician:f3dfloor_export branch from 6ecd831 to 106f3c3 Aug 4, 2019
@OrdinaryMagician

This comment has been minimized.

Copy link
Contributor Author

OrdinaryMagician commented Aug 4, 2019

I've squished it all.

@madame-rachelle

This comment has been minimized.

Copy link
Collaborator

madame-rachelle commented Aug 4, 2019

I've squished it all.

Thank you for doing that!

@coltongit

This comment has been minimized.

Copy link
Contributor

coltongit commented Aug 28, 2019

This all seems to be OK now. This can be merged if the maintainers are OK with it.

@MajorCooke

This comment has been minimized.

Copy link
Contributor

MajorCooke commented Aug 31, 2019

Hmm, Graf's requested review hasn't changed. But it seems like Marisa did what Graf wanted. Or was there something else?

@OrdinaryMagician

This comment has been minimized.

Copy link
Contributor Author

OrdinaryMagician commented Sep 5, 2019

Really want this PR merged. I found yet another situation where this is essential.

@MajorCooke

This comment has been minimized.

Copy link
Contributor

MajorCooke commented Sep 5, 2019

What would that be? You could make a demonstration of it to show graf.

@OrdinaryMagician

This comment has been minimized.

Copy link
Contributor Author

OrdinaryMagician commented Sep 6, 2019

It's impossible to detect if a map has 3d floor based swimmable water without this PR, simple as that.

@alexey-lysiuk alexey-lysiuk merged commit bcef440 into coelckers:master Sep 11, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@OrdinaryMagician OrdinaryMagician deleted the OrdinaryMagician:f3dfloor_export branch Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.