Spoiler:
[PULL REQUEST] Export FBoundingBox to ZScript
Moderator: GZDoom Developers
- phantombeta
- Posts: 2088
- 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
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.
Last edited by phantombeta on Sun Oct 20, 2019 9:24 am, edited 1 time in total.
- phantombeta
- Posts: 2088
- 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
After quite a lot of testing, this seems to be fine. It can be safely merged now.
- Graf Zahl
- Lead GZDoom+Raze Developer
- Posts: 49068
- Joined: Sat Jul 19, 2003 10:19 am
- Location: Germany
Re: [PULL REQUEST] Export FBoundingBox to ZScript
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.
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.
- drfrag
- Vintage GZDoom Developer
- Posts: 3141
- Joined: Fri Apr 23, 2004 3:51 am
- Location: Spain
- Contact:
Re: [PULL REQUEST] Export FBoundingBox to ZScript
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.
- Graf Zahl
- Lead GZDoom+Raze Developer
- Posts: 49068
- Joined: Sat Jul 19, 2003 10:19 am
- Location: Germany
Re: [PULL REQUEST] Export FBoundingBox to ZScript
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.
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.
- drfrag
- Vintage GZDoom Developer
- Posts: 3141
- Joined: Fri Apr 23, 2004 3:51 am
- Location: Spain
- Contact:
Re: [PULL REQUEST] Export FBoundingBox to ZScript
Thanks, you've used the newer BoundingRect struct. I'll try to implement it using the FBoundingBox class instead.
- Graf Zahl
- Lead GZDoom+Raze Developer
- Posts: 49068
- Joined: Sat Jul 19, 2003 10:19 am
- Location: Germany
Re: [PULL REQUEST] Export FBoundingBox to ZScript
The PR is already based on FBoundingBox, not FBoundingRect. I should probably merge those two together, they are a bit redundant.
- phantombeta
- Posts: 2088
- 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
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?
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?
- drfrag
- Vintage GZDoom Developer
- Posts: 3141
- Joined: Fri Apr 23, 2004 3:51 am
- Location: Spain
- Contact:
Re: [PULL REQUEST] Export FBoundingBox to ZScript
Yeah, i've noticed i've messed up things quite a bit. I thought it was a newer bounding box based on BoundingRect.
I'll try to merge that commit anyway.
I'll try to merge that commit anyway.
- phantombeta
- Posts: 2088
- 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
I've made the requested changes. Should be ready to merge.
- drfrag
- Vintage GZDoom Developer
- Posts: 3141
- Joined: Fri Apr 23, 2004 3:51 am
- Location: Spain
- Contact:
Re: [PULL REQUEST] Export FBoundingBox to ZScript
@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.
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.
- drfrag
- Vintage GZDoom Developer
- Posts: 3141
- Joined: Fri Apr 23, 2004 3:51 am
- Location: Spain
- Contact:
Re: [PULL REQUEST] Export FBoundingBox to ZScript
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
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
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.
- drfrag
- Vintage GZDoom Developer
- Posts: 3141
- Joined: Fri Apr 23, 2004 3:51 am
- Location: Spain
- Contact:
Re: [PULL REQUEST] Export FBoundingBox to ZScript
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.
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.
Re: [PULL REQUEST] Export FBoundingBox to ZScript
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. hehedrfrag 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?