Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor MxStreamList based lists #1260

Merged
merged 8 commits into from
Dec 23, 2024

Conversation

foxtacles
Copy link
Member

@foxtacles foxtacles commented Dec 22, 2024

I've been using roadmap and identifying pieces of code that we need to move around in order to eventually be able to have everything in its proper place. Among other things, I've noticed that the MxStreamList based lists are all defined in different compilation units. Most importantly, MxStreamListMxDSAction is defined within MxDSObject, which strongly indicates that this list does not contain MxDSAction*, but MxDSObject*. I've refactored it accordingly.

Before/after:

MxStreamList -> MxUtilityList (this is the shared base class for all of the below; I've put it in its own header for now)
MxStreamListMxDSAction -> MxDSObjectList
MxStreamListMxDSSubscriber -> MxDSSubscriberList
MxStreamListMxNextActionDataStart -> MxNextActionDataStartList

The changes should not reduce accuracy (based on my tests), but I'd appreciate a review @disinvite @jonschz

I think there's also a good chance that MxDSObject* was used in many functions in the streaming subsystem instead of MxDSAction*. However, I haven't found any hard evidence for that yet so I've left everything as-is for now, other than the places where I had to change it due to the new interfaces.

@disinvite
Copy link
Contributor

I started out working this from the BETA10 angle: whether the call to ~list, begin(),. or erase() is the same one used in other places, and thus the same template item. Is that the most vital thing to confirm for this PR? In principle I think this is a good step (and glad to see roadmap providing some insights!) so I could just give the big picture analysis for now if that's more useful.

@foxtacles
Copy link
Member Author

I started out working this from the BETA10 angle: whether the call to ~list, begin(),. or erase() is the same one used in other places, and thus the same template item. Is that the most vital thing to confirm for this PR? In principle I think this is a good step (and glad to see roadmap providing some insights!) so I could just give the big picture analysis for now if that's more useful.

I think for this PR the big picture analysis/review is sufficient - I'd mostly like a confirmation that the changes make sense to you. For instance I'm not too sure about the nature of the base class (MxUtilityList) and where we should put it for now, welcoming any suggestions.

@jonschz
Copy link
Contributor

jonschz commented Dec 23, 2024

I haven't looked too deeply into this, just some thoughts:

  • I could imagine that looking at BETA10, as @disinvite suggests, is quite fruitful here.
  • Are we sure that the MxUtilityList class exists? Is it the order in BETA10? What happens if we just duplicate the code in PopFront into every subclass?
  • I have never looked into it or attempt to find it, but could a look into the older leaked Mx source code be helpful here?

@foxtacles
Copy link
Member Author

foxtacles commented Dec 23, 2024

  • Are we sure that the MxUtilityList class exists? Is it the order in BETA10? What happens if we just duplicate the code in PopFront into every subclass?

Yes, the class exists. This is confirmed by the BETA (see MxStreamController::~MxStreamController for the best example of PopFront and the ctor of the class right before it). Of course, since they are templates, the code is duplicated and we technically don't know 100% that all the lists share a common base class, but it's reasonable to assume (technically you could just copy-paste the base class for each and you'd not be able to tell the difference in the code). Same goes for PopFront. I'm not sure there is any way we can possibly tell if it was duplicated or not, but considering it shares the exact same code I think it's reasonable to assume it's part of MxUtilityList (or whatever you'd like to call it).

I have never looked into it or attempt to find it, but could a look into the older leaked Mx source code be helpful here?

Unfortunately the 1996 leaked alpha source code is relatively useless. Most of the code is missing (or hasn't been written yet at the time), and most of what is there has been rewritten or heavily modified since. It has been useful for naming a few things, and I think about a ~dozen functions were still more or less unchanged in retail, but that's about it.

I'm going ahead with merging this - it's not necessarily the final solution to these lists, but they are in their correct locations now at least.

@foxtacles foxtacles merged commit 5b19d79 into isledecomp:master Dec 23, 2024
12 checks passed
@foxtacles foxtacles deleted the refactor-lists branch December 23, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants