Conversation
3e2522a to
be07760
Compare
8ef639b to
befb850
Compare
|
I would like to quickly have a look over the extra param I just realised GpuApi::Present has, if it may not be beneficial in some way to expose. Second and third params I believe shouldn't be important to expose but dont know too deeply what they do. After that, Ill mark it ready for review but feel free to look at it now, is a lot... |
wopss
left a comment
There was a problem hiding this comment.
Thanks. My main concern with this PR is the change to how GameStates are handled. See the comment there.
src/dll/PluginBase.cpp
Outdated
| #include "stdafx.hpp" | ||
| #include "PluginBase.hpp" | ||
| #include "Utils.hpp" | ||
| #include "stdafx.hpp" |
There was a problem hiding this comment.
This can be removed. Since it should be included automatically.
src/dll/Systems/StateSystem.hpp
Outdated
| Run(fmt::format(L"{}::OnBefore{}", state->GetName(), action->GetName()), action->onBefore, *aApp); | ||
| const auto result = ExecuteHookedAction(aHooked, aAction, aThis, aApp); | ||
| Run(fmt::format(L"{}::OnAfter{}", state->GetName(), action->GetName()), action->onAfter, *aApp); |
There was a problem hiding this comment.
This works differently than before (see https://docs.red4ext.com/mod-developers/custom-game-states#games-life-cycle). A plugin might need to block the game from switching states while it’s waiting for resources, doing extra work, or something similar.
I remember us talking about this on Discord, but I still don’t see why plugins shouldn’t be allowed to do that. Plugins are already low-level, so if avoiding CTDs is the main reason for this change, I don’t think that’s a good enough reason, since they can CTD the game with memory access.
My point with GameStates is: if you need them, then you at least know what you should do with them and when.
There was a problem hiding this comment.
I guess come to some agreement on this with @psiberx first in some way then cause you are conflicting now.
He said he feels it is overall redundant as well and agreed to this change formerly, you had not really wrote any concerns about changing it before. I dont think the way it was designed was good for just wanting to have one-off things and it definitely wasnt good enough if someone needed to hook specifically before or after for some reason.
There was a problem hiding this comment.
And there are good reasons for hooking before/after mainly when it comes to rendering.
I can make it work the way it did before with current before/after semantics but definitely wouldnt get rid of them. Can change an API to use one call and have some extra param which would say when to register, or have before/after register function, or param in the state hooks which would be different for call before/after and always call single registered hook for both... All are less optimal in practise IMO though, introducing extra redundancy or more confusing behavior.
And it does have a purpose and should be included in public API like this.
There was a problem hiding this comment.
I’m not against Pre hook, but if we’re removing a feature just to add this hook, then yeah, I am.
If we are going to remove this feature, then we can just remove the GameStates completely. I find no reason to have plugins register a game state that cannot block the game until its resources are initialized / loaded, since the Run stuff is added.
I dont think the way it was designed was good for just wanting to have one-off things and it definitely wasnt good enough
Why do you say that?
Imo, the old implementation was okayish. Depending on your state, it was executed after (BaseInit, Init, Running or before (Shutdown) the game's state.
There was a problem hiding this comment.
Actually one more good note I believe - If you just want to monitor the state, there is no good way atm to achieve that with the current API as well.
At minimum, when you would want to have something like this, you would want 3 states and not a boolean IMO - keep running until I say so, keep running until game runs it, run only once. Some int return and checking be negative/zero/positive may be better.
There was a problem hiding this comment.
Yeah Ill do it in a way that prevents repeating original state functions in such a case when Ill add it back. But I sort of stand by swapping the result typs to enum/int so there is a way to observe (probably enum would be better)
There was a problem hiding this comment.
Ok removed that comment cause right as I wrote it I realised it may be fine, Ill rather check... The comment about observing is still relevant.
There was a problem hiding this comment.
Thinking about it, we could just leave it as it is for now and create an issue to fix it later, either if someone needs it or if we decide to rework it.
About returning an enum, I fully agree.
There was a problem hiding this comment.
I agree about extended return value.
There was a problem hiding this comment.
Reworked this as a whole now, compeltely got rid of the other classes and directly hooking and fully overriding DoState and ChangeState (or whatever they are called). Ended up much cleaner IMO, directly copies what game does with the added behavior that was there before, for every single state status transition. Tested this vigorously, should work as expected with and without any hooks of any kind and combination of observes/original mechanism before changes.
|
Just some update on why nothing gets posted for a bit - I found various issues trying to use this, trying to find some better hooks for initialize/deinitialize or some combination that should be reliable enough. Initialize only does base initialization of devices, not swapchain and some other systems that I had plan to hook. Same for shutdown - is just absolute last step of shutdown as I found out and not shutdown for all systems as I thought at first. Found something already and got a bit sidetracked by it, should hopefully post some updates soon... Also considering swapping the ResizeBackbuffer for some more upper-level function as it may get called twice a frame for no good reason as I found out, leading to some suboptimal things on hooked side. |
d38a355 to
b7b60f2
Compare
15e85b8 to
ffbc1b6
Compare
afc9fe8 to
c05374d
Compare
c05374d to
d0ddf8c
Compare
|
I just quickly removed changes to SSwapChainData in RED4ext.SDK that I left in on accident and noticed just now trying to work on another PR... No other changes done, only RED4ext.SDK bump. |
RED4ext.SDKto API v1 RED4ext.SDK#178StateSystemCGameApplicationhooking