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

arg type: change type of array arguments from [n]*type -> *[n]type #85

Merged
merged 4 commits into from
Jan 18, 2023

Conversation

gucio321
Copy link
Collaborator

imo it is more logical for programmers to say just &myArray than creating a new array with pointers to each element of our array

reffer #84

@AllenDang let me know if there was any reason for doing it this way.

imo it is more logical for programmers to say just &myArray than creating a new array with pointers to each element of our array

reffer AllenDang#84
@gucio321 gucio321 requested a review from AllenDang January 12, 2023 16:52
@gucio321 gucio321 self-assigned this Jan 12, 2023
@AllenDang
Copy link
Owner

AllenDang commented Jan 13, 2023

@gucio321 It's a intuition decision, DragInt2 works with two pointer values in an array, so should be []*int, rather than a *[]int. If you did some tests, say place dragin2 with few other widgets which requires pointer type binding in same window, and they work flawlessly, I think it's ok to change the type.

@neclepsio
Copy link
Contributor

neclepsio commented Jan 13, 2023

The example should be modified to keep the same behavior as before: the DragInt and DragInt2 are no longer linked and r, g, b, a are no longer useful. The example itself demonstrates in my opinion that the change is good.

@gucio321
Copy link
Collaborator Author

@neclepsio it should work now

@AllenDang it depends on how exactly user should store values passed to cimgui.
1: Current state: dragint2 is a bundle of two diffrent properties that could come from two diffrent places (e.g. two fields of settings structure or something like that)
+ we preffer to use structures rather than hard-to-read arrays in go
- it is quit difficult and lets say... not really casual to create an array of pointers imo
2: This change: dragint2 is used to edit an array of strictly connected values... If the user would like to implement the above solution, he needs to create temporary array, pass it and update after imgui call.
+ easier to crete in code
- could be not flexible enough for go realities

Personally, after re-analysing the situation, I think that the current scenerio is good, but the second may be more universal.... I don't know what do do tbh 😄

@AllenDang you're the boss here, please, decide what to do 🙃

@AllenDang
Copy link
Owner

@gucio321 I agree your solution, *[]type is better

@gucio321
Copy link
Collaborator Author

ok, so lets go ahead and change this!

@gucio321
Copy link
Collaborator Author

okey, @AllenDang it is done now

Copy link
Owner

@AllenDang AllenDang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@AllenDang AllenDang merged commit edbcf7e into AllenDang:main Jan 18, 2023
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