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

Clarify that some exclude properties of physics query parameters are copied #94059

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

ayanchavand
Copy link
Contributor

Fixes #93895

  • Added the line Note that [code]exclude[/code] is a replace-don't-edit type property and can only be modified by replacing the entire array. to the PhysicsShapeQueryParameters3D.exclude docs

@ayanchavand ayanchavand requested a review from a team as a code owner July 8, 2024 05:09
@AThousandShips AThousandShips changed the title Added exclude's replace-don't-edit type property in PhysicsShapeQueryParameters3D docs Add exclude's replace-don't-edit type property in PhysicsShapeQueryParameters3D docs Jul 8, 2024
@AThousandShips AThousandShips added enhancement documentation cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Jul 8, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Jul 8, 2024
@AThousandShips AThousandShips changed the title Add exclude's replace-don't-edit type property in PhysicsShapeQueryParameters3D docs Clarify that PhysicsShapeQueryParameters3D.exclude is copied Jul 8, 2024
@AThousandShips
Copy link
Member

AThousandShips commented Jul 8, 2024

This should probably be done to all the cases of this, both 2D and 3D as well as the other parameters such as point and ray queries

Edit: to clarify, I don't mean any case at all, but the physics classes, i.e.:

  • PhysicsShapeQueryParameters2D
  • PhysicsPointQueryParameters2D
  • PhysicsPointQueryParameters3D
  • PhysicsRayQueryParameters2D
  • PhysicsRayQueryParameters3D

Including some that exclude objects by int

@Mickeon
Copy link
Contributor

Mickeon commented Jul 8, 2024

Your own PR does this but it is done for every Packed Array property. To handles Arrays like these would require a solution that allows properties to add the note automatically given a certain criteria.

@AThousandShips
Copy link
Member

AThousandShips commented Jul 8, 2024

That would be pretty complex as there's no way to detect it, so it'd be a matter of adding a new property to the XML class, not sure it's worth it, but might look into checking how many cases there are for this

There's no way to detect this automatically, but we could add a hint or something to make the doc generation add it automatically to the XML, but I don't think it's common enough to justify it

@Mickeon
Copy link
Contributor

Mickeon commented Jul 8, 2024

Not this case specifically, but there are many times where information is repeated that may benefit from such a system, similar to the keywords.

@AThousandShips
Copy link
Member

Absolutely, and I'll look at if there's enough cases to justify a system, but since this isn't automatable, and is likely not very common (i.e. cases that copy and aren't Packed*Arrays which require a different message), it might not be worth it, but I'll take a look at adding an option to manually tag with is_copied or something to make the output text automated

@AThousandShips
Copy link
Member

Please add to the other cases mentioned to get this to cover all the related cases, then this should be ready!

@AThousandShips
Copy link
Member

You also merged your branch instead of using a force push, please use git push --force instead, and when you're done please squash your commits into one, see here

@ayanchavand
Copy link
Contributor Author

I think I accidentally created a branch from some other local branch instead of master, is it okay if I open new pr with the above changes?

@AThousandShips AThousandShips changed the title Clarify that PhysicsShapeQueryParameters3D.exclude is copied Clarify that some exclude properties of physics query parameters are copied Jul 8, 2024
@AThousandShips
Copy link
Member

You can just rebase it, see the link, it shouldn't matter as long as you rebase on master

Also please make the final commit message something clear, like the PR title

@ayanchavand
Copy link
Contributor Author

oh got it, thank you and sorry for the inconvenience

@AThousandShips
Copy link
Member

No inconvenience at all! Part of what I'm here for is to help along!

@ayanchavand ayanchavand force-pushed the fix-exclude-docs branch 2 times, most recently from c89f02b to e73a193 Compare July 8, 2024 12:57
@AThousandShips
Copy link
Member

AThousandShips commented Jul 8, 2024

After fixing the incorrect line above please update the commit title (using git commit --amend), and you can remove the co-author it's not needed for suggestions

(The title still hasn't been changed, it should be "Clarify that some exclude properties of physics query parameters are copied" instead of "Added exculde's replace-don't-edit type property in docs")

@ayanchavand ayanchavand force-pushed the fix-exclude-docs branch 2 times, most recently from 19e775b to 4a631d4 Compare July 8, 2024 13:05
@ayanchavand
Copy link
Contributor Author

done

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Jul 9, 2024
@akien-mga akien-mga removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Jul 9, 2024
@akien-mga akien-mga merged commit 73422df into godotengine:master Jul 10, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release documentation enhancement
Projects
None yet
5 participants