fezhub / DSP-Mods

Dyson Sphere Program Mods
GNU Lesser General Public License v2.1
43 stars 11 forks source link

Support for rotation, fix some visual glitches, and add build previews. #11

Closed colin-daniels closed 3 years ago

colin-daniels commented 3 years ago

There's probably some overlap with #10, but this takes all the deltas/etc and makes them relative to the source building rotation, which adds rotation support in the process. Feel free to cannibalize this or whatever you like.

Note that seems to partially fix some of the visual glitches as well, at least for my build, which I think were in part due to the special logic for reversing deltas when inserters were incoming.

colin-daniels commented 3 years ago

Seems to fix #8 as well, which makes sense since it's rotation related. Note that copied inserters are still a little bit off versus the original building, but I imagine that's what #10 fixes.

image

colin-daniels commented 3 years ago

Merged #10 and #12 to keep history/contribution info.

brokenmass commented 3 years ago

Thank you, good idea. I'm going to close #12.

brokenmass commented 3 years ago

next step: instead of removing the previews in the "CreatePrebuildsPrefix" patch, it should be possible to just create the correctly position the previews using the code of "NotifyBuiltPrefix" thus improving the overall experience.

outstanding issue: direct insertion inserter are faster than expected.

Suggestion: can we move this repo (and any other modes might want to join) into a shared GitHub organisation ?

colin-daniels commented 3 years ago

@brokenmass Seems like there's an issue with PlayerAction_BuildCheckBuildConditionsPostfix if you copy a building that has an inter-building inserter on it. image

Edit: Nevermind, just made a quick fix. Not sure why it's happening though...

daviscook477 commented 3 years ago

this also breaks support for the advanced build mod that lets you place multiple things at once using alt.

daviscook477 commented 3 years ago

but I can't seem to figure out why.

daviscook477 commented 3 years ago

@colin-daniels this seems to fix the incompatibility with advanced building:

/// <summary>
            /// After player designates a PreBuild, check if it is an assembler that has Cached Inserters
            /// </summary>
            [HarmonyPostfix]
            [HarmonyPatch(typeof(PlayerAction_Build), "AfterPrebuild")]
            public static void PlayerAction_BuildAfterPrebuildPostfix(PlayerAction_Build __instance, ref PlanetFactory ___factory, PlanetAuxData ___planetAux)
            {
                // Do we have cached inserters?
                var ci = PatchCopyInserters.cachedInserters;
                if (ci.Count > 0)
                {
                    foreach (var buildPreview in __instance.buildPreviews)
                    {
                        Vector3 targetPos;
                        Quaternion targetRot;
                        if (__instance.buildPreviews.Count > 1)
                        {
                            targetPos = buildPreview.lpos;
                            targetRot = buildPreview.lrot;
                        }
                        else
                        {
                            targetPos = __instance.previewPose.position + __instance.previewPose.rotation * buildPreview.lpos;
                            targetRot = __instance.previewPose.rotation;
                        }
                        var entityPool = ___factory.entityPool;
                        foreach (var inserter in ci)

the code that uses the previewPose only works correctly when there's only one build preview. but it seems that using the build preview pose information only works properly if there's more than one build preview. so that's why it needs two cases.

colin-daniels commented 3 years ago

@daviscook477 Tested + seems to work, thanks.

image image

colin-daniels commented 3 years ago

By the way @fezhub, you should be able to merge #10 before this one if you want to do it in two stages. Worst case scenario, I'll just have to rebase + fight with git a bit (but I'm pretty sure it won't be an issue since I did a merge earlier).

daviscook477 commented 3 years ago

If you save the stt on the cached inserter and then add the following lines to the NotifyBuilt patch it will properly set the length of the copied inserters. right now the length is always set to 1 which makes them work faster than they are supposed to if the original sorter was actually 2 or 3 pips away from the building.

// Set the speed of the inserter
                                ItemProto itemProto = LDB.items.Select(pi.ci.protoId);
                                pbdata.refCount = Mathf.RoundToInt((float) (pi.ci.stt - 0.499000012874603) / itemProto.prefabDesc.inserterSTT);

@colin-daniels As far as I can tell it seems like in this case the devs are just using the refCount field on the prebuilddata as just a data field to store the distance from the belt to the machine. Despite the name not making any sense that is in fact that field that is used to send that data to the code where inserter components are made (see PlanetFactory.cs method CreateEntityLogicComponents at if (desc.isInserter) if you are curious)

colin-daniels commented 3 years ago

@daviscook477, feel free to commit the changes to the main branch in my fork if you think they should be in this PR (I added you and @brokenmass as collaborators if needed). I'm getting a little nervous about how big this PR is getting though, seems to be never ending lol

brokenmass commented 3 years ago

@daviscook477, feel free to commit the changes to the main branch in my fork if you think they should be in this PR (I added you and @brokenmass as collaborators if needed). I'm getting a little nervous about how big this PR is getting though, seems to be never ending lol

Yes this PR is getting huge. @fezhub: any chance you can review and merge this soon?

brotchie commented 3 years ago

@colin-daniels Really nice work on the ReversePatch of DetermineBuildPreviews to lift the sorter posing into a function.

colin-daniels commented 3 years ago

@colin-daniels Really nice work on the ReversePatch of DetermineBuildPreviews to lift the sorter posing into a function.

I'm afraid I can take exactly zero credit on that one 😛, most of the significant functionality changes are by @daviscook477 and @brokenmass. Check out the individual commits if you're interested in something specific.

brokenmass commented 3 years ago

additional functionalities:

fezhub commented 3 years ago

Testing results, this looks great, just minor things that we can probably ignore; image image rotated buildings either don't connect or connect backwards, personally this feels like a wont-fix but there's probably some use case i'm not imagining where it ruins someones day.. let's wait and see? image tried a stress test with pole placed chemlabs, the inserters overlap here. again can this be a wont-fix?

colin-daniels commented 3 years ago

Testing results, this looks great, just minor things that we can probably ignore; image image rotated buildings either don't connect or connect backwards, personally this feels like a wont-fix but there's probably some use case i'm not imagining where it ruins someones day.. let's wait and see? image tried a stress test with pole placed chemlabs, the inserters overlap here. again can this be a wont-fix?

The rotation thing is due to vanilla behavior where if you copy a building that was placed in a rotated manner, the build preview resets to the original rotation. It's just less obvious with smelters versus something like a railgun. I'd call it a wont-fix. ezgif-7-40d3185e0b8d

I've noticed the overlap thing as well, but yeah that's pretty minor issue all things considered. @brokenmass might know more about how easy it would be to fix that (since he put in the code for inserter previews + made them ignoreCollider presumably for a reason).