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

Gemvertexbuffer fix #2

Closed
wants to merge 4 commits into from

Conversation

avilleret
Copy link
Contributor

fix issue : #1 #1

here is the up-to-date patch :
--- START ---

N canvas 785 377 1084 672 10;

X msg 792 483 draw line;

X msg 607 287 position XYZ , color color;

X obj 580 261 t a b;

X obj 580 36 gemhead;

X obj 791 456 loadbang;

X obj 208 226 loadbang;

X obj 383 402 table XYZ 9;

X obj 382 378 table color 12;

X msg 222 292 ; color const 1;

X msg 171 348 1 , 1 , 0 , -1 , 1 , 0 , 0.2 , 0.5 , 0;

X obj 171 314 t b b;

X obj 171 381 t a b;

X obj 201 416 f;

X obj 231 415 + 1;

X obj 171 446 tabwrite XYZ;

X msg 216 377 0;

X msg 695 365 resize 3;

X msg 696 428 resize $1;

X floatatom 696 409 5 0 0 0 - - -, f 5;

X obj 579 575 gemvertexbuffer 3;

X text 728 409 increase VBO size to display all points;

X obj 394 189 gemwin;

X msg 411 139 destroy;

X msg 394 108 color 0 0 1 , create , 1;

X msg 649 329 resize 12 , partial_draw 0 3;

X text 823 328 click here to fix;

X connect 0 0 19 0;

X connect 1 0 19 0;

X connect 2 0 19 0;

X connect 2 1 1 0;

X connect 3 0 2 0;

X connect 4 0 0 0;

X connect 5 0 8 0;

X connect 5 0 10 0;

X connect 9 0 11 0;

X connect 10 0 9 0;

X connect 10 1 15 0;

X connect 11 0 14 0;

X connect 11 1 12 0;

X connect 12 0 13 0;

X connect 12 0 14 1;

X connect 13 0 12 1;

X connect 15 0 12 1;

X connect 16 0 19 0;

X connect 17 0 19 0;

X connect 18 0 17 0;

X connect 22 0 21 0;

X connect 23 0 21 0;

X connect 24 0 19 0;

--- END ---

Signed-off-by: Antoine Villeret <antoine.villeret@gmail.com>
Signed-off-by: Antoine Villeret <antoine.villeret@gmail.com>
Signed-off-by: Antoine Villeret <antoine.villeret@gmail.com>
@umlaeute
Copy link
Owner

umlaeute commented Jan 7, 2014

how does this fix issue#1 ?

afaict the patch doesn't really fix the original issue, but provides a workaround to deal with it. correct?
wouldn't it be possible to really fix the issue instead (e.g. checking the table dimension and clamping the possible resize values accordingly?

related: what happens if you resize -1?

also, isn't the help-patch updated to include the new partial_draw message?

@avilleret
Copy link
Contributor Author

2014/1/7 umläute notifications@github.com

how does this fix issue#1 ?

sorry, it doesn't in fact, it's just a workaround...

afaict the patch doesn't really fix the original issue, but provides a
workaround to deal with it. correct?

yes

wouldn't it be possible to really fix the issue instead (e.g. checking
the table dimension and clamping the possible resize values accordingly?

I think it's possible, but I don't understand the problem this is why I
choose the workaround instead of a fix (and sorry for the confusion)
checking the size and clamping it is another workaround, isn't it ?
and I'm not sure clamping is enough, maybe the VBO should be a little bit
larger than the table,
at this point I don't understand what happens, my OpenGL knowledge is too
limited I think...

related: what happens if you [resize -1(?

it is clamped to 1 : vbo_size = size>1?size:1; on line 351 of
gemvertexbuffer.cpp

also, isn't the help-patch updated to include the new partial_drawmessage?

should be on commit
5ac7169avilleret@5ac7169
if
I'm not wrong


Reply to this email directly or view it on GitHubhttps://github.com//pull/2#issuecomment-31750382
.

@umlaeute
Copy link
Owner

umlaeute commented Jan 8, 2014

On 2014-01-07 19:24, Antoine Villeret wrote:

related: what happens if you [resize -1(?

it is clamped to 1 : vbo_size = size>1?size:1; on line 351 of
gemvertexbuffer.cpp

ah, there was a bug in my template code which would result in [resize -1( effectively calling the resize() method with 0xFFFFFFFF as the
argument, which is >1 and crashes either Gem (if you are lucky) or the
system (if you are not) or nothing at all (if you have plenty of RAM).

fixed with 42a48c3

fgmasrd
IOhannes

@umlaeute
Copy link
Owner

umlaeute commented Jan 8, 2014

On 2014-01-07 19:24, Antoine Villeret wrote:

wouldn't it be possible to really fix the issue instead (e.g. checking
the table dimension and clamping the possible resize values accordingly?

I think it's possible, but I don't understand the problem

turns out that the code for copying interleaved array data was slightly
bogus.
hopefully i've fixed it with fd169f7.

please confirm.
does the fix make this pull request redundant?

fgmadsr
IOhannes

@avilleret
Copy link
Contributor Author

2014/1/8 umläute notifications@github.com

On 2014-01-07 19:24, Antoine Villeret wrote:

wouldn't it be possible to really fix the issue instead (e.g.
checking
the table dimension and clamping the possible resize values
accordingly?

I think it's possible, but I don't understand the problem

turns out that the code for copying interleaved array data was slightly
bogus.
hopefully i've fixed it with fd169f7.

please confirm.

yes it's fixed , thanks for that

does the fix make this pull request redundant?

yes and no...
my workaround adds the possibility to not draw all the VBO without resizing
(as a side effect)
and I think this feature could be useful while it's not efficient to resize
VBO (or table) at each frame

so I will keep the partial_draw message (it can be rename also...) as a new
feature

cheers

a

fgmadsr
IOhannes


Reply to this email directly or view it on GitHubhttps://github.com//pull/2#issuecomment-31846921
.

@umlaeute
Copy link
Owner

umlaeute commented Jan 8, 2014

On 2014-01-08 17:49, Antoine Villeret wrote:

does the fix make this pull request redundant?

yes and no...
my workaround adds the possibility to not draw all the VBO without resizing
(as a side effect)
and I think this feature could be useful while it's not efficient to resize
VBO (or table) at each frame

so I will keep the partial_draw message (it can be rename also...) as a new
feature

ok, could you prepare the pull-request so it applies cleanly to current
master? (i haven't checked)

about the name: i'm sure there is a better one (but cannot think of one
now)... changing the name should be pretty easy (if it happens soon enough)

fgmadsr
IOhannes

@umlaeute umlaeute closed this Jan 9, 2014
avilleret referenced this pull request in avilleret/Gem Jan 21, 2014
umlaeute added a commit that referenced this pull request Aug 26, 2014
e.g. if mouse #2 moves to 812/432, we output
 "mouse 2 motion 812 432"
rather than
 "mouse motion 2 812 432"

also output full list (last element was missing)
@ch-nry ch-nry mentioned this pull request Dec 4, 2014
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.

None yet

2 participants