Page 1 of 2

[PULL REQUEST] Export FBoundingBox to ZScript

Posted: Thu Dec 06, 2018 9:32 am
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:

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Posted: Sat Dec 08, 2018 12:38 am
by phantombeta
After quite a lot of testing, this seems to be fine. It can be safely merged now.

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Posted: Sat Dec 08, 2018 4:43 am
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.

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Posted: Sat Dec 08, 2018 5:38 am
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. :(

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Posted: Sat Dec 08, 2018 6:10 am
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.

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Posted: Sat Dec 08, 2018 11:57 am
by drfrag
Thanks, you've used the newer BoundingRect struct. I'll try to implement it using the FBoundingBox class instead.

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Posted: Sat Dec 08, 2018 12:35 pm
by Graf Zahl
The PR is already based on FBoundingBox, not FBoundingRect. I should probably merge those two together, they are a bit redundant.

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Posted: Sat Dec 08, 2018 12:38 pm
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?

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Posted: Sat Dec 08, 2018 12:50 pm
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.

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Posted: Sat Dec 08, 2018 1:12 pm
by phantombeta
I've made the requested changes. Should be ready to merge.

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Posted: Sun Dec 09, 2018 3:07 pm
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.

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Posted: Mon Dec 10, 2018 5:42 am
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

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Posted: Mon Dec 10, 2018 6:52 am
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.

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Posted: Tue Dec 11, 2018 4:55 am
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.

Re: [PULL REQUEST] Export FBoundingBox to ZScript

Posted: Tue Dec 11, 2018 6:21 am
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