Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Burst projectile retargeting #1073
base: develop
Are you sure you want to change the base?
Burst projectile retargeting #1073
Changes from 16 commits
dc865b2
1506095
60cbebf
82dbb40
9d7b854
ba2e450
a364fe9
ef93c81
1ea2d23
2fd3455
ab1cb83
24b2cc1
6f85473
68296f9
da32f58
ea7d0e3
f32981b
d9e8c2a
12ffb72
54f327e
f83e32f
24e0606
9336b6c
d84a418
1692452
d4d841c
e4a625b
a6ea99a
bb09eb0
5677163
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
GetRandomTarget
function is also quite complex. Similar to theUpdateRandomTarget
function, consider breaking it down into smaller functions for better readability and maintainability. For example, the part where it checks if the candidate is valid could be a separate function. This would make the code easier to understand and maintain.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am also not sure what does this function do and why is it in ScriptExt:
ScriptExt::IsUnitMindControlledFriendly(pThis->Owner, pCandidate)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Metadorius The function
ScriptExt::IsUnitMindControlledFriendly(pThis->Owner, pCandidate)
is likely used to check if a unit (pCandidate
) is mind-controlled and is friendly to the owner (pThis->Owner
).The reason it's in
ScriptExt
might be because it's a utility function that can be used in various scripts, not just in this context. It's common to place such utility functions in a separate class or namespace (likeScriptExt
) to avoid code duplication and improve code organization.However, without more context or access to the
ScriptExt
class, this is just an educated guess. You might want to check the implementation ofScriptExt::IsUnitMindControlledFriendly
for a more accurate understanding.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function
ScriptExt::IsUnitMindControlledFriendly()
was originally placed inScriptExt
because that is the only place where it was called. If it is now being called elsewhere it should likely be relocated toTechnoExt
since that would be more appropriate for function that checksTechnoClass
state. Same forScriptExt::IsUnitAvailable()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually place static extension functions as if they were the non-static functions of the class that is being extended (think of C# extension methods), so if it accepts the TechnoClass, it should be in TechnoExt. I am not sure if the description you gave is accurate, I would say that it needs a clarification from @FS-21.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean ScriptExt::IsUnitMindControlledFriendly()? I don't touch what other people do, like that ScriptExt::IsUnitAvailable().
I'll move them to TechnoExt then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The serialization of
OriginalTarget
andCurrentRandomTarget
is added, but there is no version check to ensure backward compatibility with save files that do not have these fields. This could lead to issues when loading older save files. Implement a version check to handle the absence of these fields in older save files.