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

hard_rock method should be idempotent #80

Closed
tybug opened this issue Dec 14, 2020 · 6 comments · Fixed by #82
Closed

hard_rock method should be idempotent #80

tybug opened this issue Dec 14, 2020 · 6 comments · Fixed by #82

Comments

@tybug
Copy link
Collaborator

tybug commented Dec 14, 2020

I have a scenario where I want to ensure that a hitobject is its hard_rock version, but I can't control where that hitobject comes from, and so have no way of knowing if I am being passed an already hard_rock-ified hitobject, or a normal hitobject. The cleanest solution imo would be to prevent multiple applications from doing anything.

@llllllllll will implement this if you give the go-ahead, or maybe you have another solution in mind

@llllllllll
Copy link
Owner

I think this is okay; however, currently most things don't actually care about this, just the attributes, so I am wondering what the use case is.

If you do end up doing this, we should do the same with EZ, HT, and DT

@tybug
Copy link
Collaborator Author

tybug commented Dec 14, 2020

not sure what you mean about most things caring about the attributes, I want the x and (especially for hr) y position to be correct. I have a method (https://github.com/circleguard/circlecore/blob/master/circleguard/hitobjects.py#L30) which takes a hitobject. Some usages call it with an unmodified hitobject, and some call it with a hitobj.hard_rock hitobject.

I then call hard_rock on it if hr is enabled in the replay, which incorrectly reverts it back to its normal state if it was already hr, and correctly turns it to hr if it was normal before. I would prefer to be able to accept both and not require that only one type is passed in.

@jxu
Copy link
Contributor

jxu commented Dec 14, 2020

my proposal from long ago was to have hitobject class have the Mod as attribute with Mod being its own class. I do not like all the mod parameters being passed in one at a time. But i haven't looked at the code in months so feel free to ignore what I say.
#61

@llllllllll
Copy link
Owner

I generally believe functions should only take the state which actually matters to them. I thought that using a Mod object would make it hard to tell which mods actually influenced different areas of the code. For example, code that deals with the time of a hit object doesn't need most mods; just DT/HT. I do think that the current solution is really tedious, so I would be open to making a ModSet object, which may just be bitmask internally, that represents a set of active mods.

@jxu
Copy link
Contributor

jxu commented Dec 14, 2020

If you really desire the separation there could be different mod subclasses but I think treating all mods together is reasonable

@llllllllll
Copy link
Owner

I agree that we should put all the mods together if we do group them. I think I err on the side of more state separation instead over too much grouping. Once the abstractions settle it is easy to group things back together, and keeping things split makes testing easy. It is much harder to break something up because the dependencies are hidden, and in tests you may need to provide more mocked state that actually required. In this case with mods, I don't think those are issues anymore

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 a pull request may close this issue.

3 participants