[PULL REQUEST] Export FBoundingBox to ZScript

Moderator: GZDoom Developers

User avatar
phantombeta
Posts: 2084
Joined: Thu May 02, 2013 1:27 am
Operating System Version (Optional): Windows 10
Graphics Processor: nVidia with Vulkan support
Location: Brazil

[PULL REQUEST] Export FBoundingBox to ZScript

Post by phantombeta »

I've closed this PR a while ago. The code has issues and this is a pretty bad way to do it. For others who make PRs, please don't ever try to make any new PRs for this code.
Spoiler:
Last edited by phantombeta on Sun Oct 20, 2019 9:24 am, edited 1 time in total.
User avatar
phantombeta
Posts: 2084
Joined: Thu May 02, 2013 1:27 am
Operating System Version (Optional): Windows 10
Graphics Processor: nVidia with Vulkan support
Location: Brazil

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Post by phantombeta »

After quite a lot of testing, this seems to be fine. It can be safely merged now.
User avatar
Graf Zahl
Lead GZDoom+Raze Developer
Lead GZDoom+Raze Developer
Posts: 49056
Joined: Sat Jul 19, 2003 10:19 am
Location: Germany

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Post by Graf Zahl »

Like your other PR, please consider direct native functions when adding VM exports.

I can already tell that in the future I won't merge PRs which export functions and not provide these. And also, please try to keep the VM exports out of the main code. It was a major hassle to go through the main files once and move the majority of these to the side where they don't get in the way.
User avatar
drfrag
Vintage GZDoom Developer
Posts: 3141
Joined: Fri Apr 23, 2004 3:51 am
Location: Spain
Contact:

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Post by drfrag »

And this cannot be merged into the legacy build, i could not merge "- changed the stencil cap drawer to only cover the area which is actually used by the portal." That uses sections and i don't know how to implement the bounding box in a different way with existing old code. I really need some hints on how to do it/some help. :(
User avatar
Graf Zahl
Lead GZDoom+Raze Developer
Lead GZDoom+Raze Developer
Posts: 49056
Joined: Sat Jul 19, 2003 10:19 am
Location: Germany

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Post by Graf Zahl »

In that case you'll have to manually merge the parts you need. What do you need from that commit anyway? FBoundingBox is old stuff.

But don't be surprised. It will get increasingly difficult to keep the code base up to date. We all knew that the vintage build was merely a stopgap measure to delay the inevitable. If we can get one or two more releases out it would be long enough to build a graceful exit strategy. I guess once the texture manager rework I started this week is done there will be no way back, because that will inevitably dig deep into the GL renderer as well when I seriously start fixing all the deficiencies in there which are owed to a container that was singlemindedly written for the software renderer.

I'll be putting this one on hold until you have sorted the bounding box out.
User avatar
drfrag
Vintage GZDoom Developer
Posts: 3141
Joined: Fri Apr 23, 2004 3:51 am
Location: Spain
Contact:

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Post by drfrag »

Thanks, you've used the newer BoundingRect struct. I'll try to implement it using the FBoundingBox class instead.
User avatar
Graf Zahl
Lead GZDoom+Raze Developer
Lead GZDoom+Raze Developer
Posts: 49056
Joined: Sat Jul 19, 2003 10:19 am
Location: Germany

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Post by Graf Zahl »

The PR is already based on FBoundingBox, not FBoundingRect. I should probably merge those two together, they are a bit redundant.
User avatar
phantombeta
Posts: 2084
Joined: Thu May 02, 2013 1:27 am
Operating System Version (Optional): Windows 10
Graphics Processor: nVidia with Vulkan support
Location: Brazil

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Post by phantombeta »

I'm wondering something: To actually be able to export it, I had to define a new class named "DBoundingBox", which inherits from both FBoundingBox and DObject. This is due to the fact that FBoundingBox doesn't inherit from DObject.
Before doing that, I actually tried to export FBoundingBox itself by making it inherit from DObject, but since it's created and destructed everywhere, this just made GZDoom complain that it was being destroyed outside the GC.
Is this the correct way to export it, or is there some way to export FBoundingBox itself?
User avatar
drfrag
Vintage GZDoom Developer
Posts: 3141
Joined: Fri Apr 23, 2004 3:51 am
Location: Spain
Contact:

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Post by drfrag »

Yeah, i've noticed i've messed up things quite a bit. I thought it was a newer bounding box based on BoundingRect. :oops:
I'll try to merge that commit anyway.
User avatar
phantombeta
Posts: 2084
Joined: Thu May 02, 2013 1:27 am
Operating System Version (Optional): Windows 10
Graphics Processor: nVidia with Vulkan support
Location: Brazil

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Post by phantombeta »

I've made the requested changes. Should be ready to merge.
User avatar
drfrag
Vintage GZDoom Developer
Posts: 3141
Joined: Fri Apr 23, 2004 3:51 am
Location: Spain
Contact:

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Post by drfrag »

@Graf Zahl:
And still "changed the stencil cap drawer to only cover the area which is actually used by the portal" cannot be merged, i've added the Contains methods to FBoundingBox (it had only inRange which requires a line and i need the camera position). But i also need AllocVertices in DrawPortalStencil and it's in HWDrawInfo not in FFlatVertexBuffer.
User avatar
drfrag
Vintage GZDoom Developer
Posts: 3141
Joined: Fri Apr 23, 2004 3:51 am
Location: Spain
Contact:

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Post by drfrag »

And surprisingly i've managed to merge it and apparently it's working now but i don't know why, may be it's just pure luck. :)
AllocVertices was in FDrawInfo in that revision. I tested with thebox.wad.

I've reverted it for now, the following two commits need reviewing.
I applied "- fixed: Horizon portals must be drawn in the context of their containing drawinfo" in the middle of them.
https://github.com/drfrag666/gzdoom/com ... 07cb6ed116
https://github.com/drfrag666/gzdoom/com ... 287c0f70f0
User avatar
Rachael
Posts: 13531
Joined: Tue Jan 13, 2004 1:31 pm
Preferred Pronouns: She/Her
Contact:

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Post by Rachael »

Be careful about committing directly to your legacy branch without creating a test branch, if indeed you merged this PR. That's going to create another 3.5.1-like situation where you had some unreleased 3.6.0 features in your release build.
User avatar
drfrag
Vintage GZDoom Developer
Posts: 3141
Joined: Fri Apr 23, 2004 3:51 am
Location: Spain
Contact:

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Post by drfrag »

You shouldn't be too worried about that commit after i merged over 400 commits in a couple of days. :)
But it's not the PR, it's a bugfix for the dot in the sky: viewtopic.php?f=7&t=62518

I merged it becouse i wanted to prove myself i was capable of doing it, it was not important. I could have created a test branch but instead i've reverted it for now until it's reviewed. BTW what do you think of those commits?
@Graf:
What i did (besides using FBoundingBox and adding the Contains methods) was adding the FDrawInfo *di parameter to GLPortal::DrawPortalStencil and use it in the GLPortal::Start calls, in GLPortal::End i called it with a null pointer instead and inside the method i used the old code with STENCILTOP and STENCILBOTTOM indexes if di is null.
I don't even know how stencil works and i'm not sure if that makes much sense but the test wad works well. :)

What happened with 3.5.1 is that i didn't notice some commits in there were not in the main point release until a moment before but those did not introduce bugs (i reviewed them) and i was already aware of the problem with settings controllers but i chose not to revert it for that legacy release.
Last edited by drfrag on Tue Dec 11, 2018 12:51 pm, edited 1 time in total.
User avatar
Rachael
Posts: 13531
Joined: Tue Jan 13, 2004 1:31 pm
Preferred Pronouns: She/Her
Contact:

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Post by Rachael »

drfrag wrote:You shouldn't be too worried about that commit after i merged over 400 commits in a couple of days. :)
But it's not the PR, it's a bugfix for the dot in the sky: viewtopic.php?f=7&t=62518

I merged it becouse i wanted to prove myself i was capable of doing it, it was not important. I could have created a test branch but instead i've reverted it for now until it's reviewed. BTW what do you think of those commits?
Actually - if it's a bugfix and it's not known or likely to cause any other issues, it should be unconditionally pushed. My mistake. You probably should revert that revert. :) hehe
Post Reply

Return to “Closed Feature Suggestions [GZDoom]”