Bug 702

mass reassign to dri-devel@, i'm no longer the default component owner.  sorry
for the spam.

close, we have aliased points in mesa now, and aa points are probably not far
behind.

(In reply to comment #2)
> close, we have aliased points in mesa now, and aa points are probably not far
> behind.
That's not quite true, support for large points was added to r200, but not
radeon. The radeon driver definitely would need to emulate large points with
tris in contrast to the r200 which can do it natively with its point sprite
primitive. (That said, it may not be that important nowadays. Things like
blender have been fixed to not expect large point size support.)

Created attachment 40069 [details]
Screenshot of modified "point" program from mesa-demos on r200

Created attachment 40070 [details]
Screenshot of modified "point" program from mesa-demos on i915

This bug still exists on r200, and possibly Radeon generally.

I've modified mesa-demos-8.0.1/src/trivial/point.c with a glPointSize(16) call right before glBegin(GL_POINTS), and attached screenshots of the window it produces on both r200 and i915. The latter is correct, the former illustrates the bug. I get proper output using Nvidia's driver as well.

This bug is affecting Wings3D for me similarly to how it affects Maya for the original reporter; the program cannot display visible vertex points on mesh wireframes, and isolated vertices are all but impossible to see.

This behavior is inconsistent with other DRI drivers, and is particularly egregious on hardware (ATI FireGL 8800) that was purpose-built for CAD applications. This is not a request for enhancement, but a straight-up bug.

I am running Ubuntu Maverick on amd64 with kernel 2.6.36 plus Alex Deucher's patch from bug #25544, and Mesa packages from Ubuntu's xorg-edgers/radeon PPA built from git 20101103.

(In reply to comment #6)
> This bug still exists on r200, and possibly Radeon generally.
I am quite sure this used to work on r200 (not r100 which can't do it natively). Aliased points should be supported up to size 2047 (though antialiased points are still limited to size 1). What does glxinfo -l say wrt point size?

Hmm actually I see the problem. The driver relies on ctx->_TriangleCaps and tests for DD_POINT_SIZE. But nothing ever sets this - update_tricaps would but it's ifdefed out as it was basically reverted, looks like the revert wasn't quite complete.
Note that it should still work if point attenuation is used (e.g. pointblast demo).

Created attachment 40078 [details] [review]
trivial possible fix

Here's a simple patch which would restore previous (2006...) behaviour...
Though I'm thinking probably the driver should just do this on its own depending on the ctx->Point.Size.
Note the problem is the hardware has two hw primitives for points: point sprite and point. Only the former can have size larger than 1. I'm not quite sure (or can't remember) why we're not always using point sprites but points when the size is 1 and it's not attenuated. Maybe it's faster.

Hi Roland, I see you've been here before!

I no longer have the Ubuntu Maverick install I was testing with earlier (I don't have the disk it was on anymore), but I do have here an Ubuntu Lucid install (same hardware) with Mesa 7.7.1, which gives me this:

$ glxinfo -l | grep -i point
    GL_ARB_occlusion_query, GL_ARB_point_parameters, GL_ARB_point_sprite, 
    GL_EXT_packed_pixels, GL_EXT_point_parameters, GL_EXT_polygon_offset, 
    GL_ALIASED_POINT_SIZE_RANGE = 1, 2047
    GL_SMOOTH_POINT_SIZE_RANGE = 1, 1

That seems to agree with what you said.

Your patch applies cleanly to 7.7.1, and indeed, it makes the points big! The modified "point" demo, "pointblast" (only w/"Point smooth off"), and Wings3D all now do as they should. Should I test your patch with bleeding-edge code?

I don't know what to make of the patch tweaking generic code instead of driver code, but it surely solves the problem for me. Could this, or some form of this, be committed into the tree?

(In reply to comment #10)
> Your patch applies cleanly to 7.7.1, and indeed, it makes the points big! The
> modified "point" demo, "pointblast" (only w/"Point smooth off"), and Wings3D
> all now do as they should. Should I test your patch with bleeding-edge code?
That should still work the same.

> 
> I don't know what to make of the patch tweaking generic code instead of driver
> code, but it surely solves the problem for me. Could this, or some form of
> this, be committed into the tree?
I think either this should be commited or instead it should be handled by the driver and the DD_POINT_SIZE flag completely removed as it's currently just broken. Since the plan initially was to remove the whole _TriangleCaps stuff and r200 is the only driver which makes use of this particular flag I'm leaning towards the latter.

In fact, the driver is quite broken anyway wrt point size, since with vertex programs you could output the point size per vertex, but the driver might still use the 1-sized point primitive, since _TriangleCaps DD_POINT_SIZE only looks at the global point size. But I really have no idea why the driver tries to use the point primitive instead of point sprite for 1-pixel points, otherwise it would be easiest to just always use point sprite prim (at least for aa points) which would get rid of both bugs.
(The other DD_POINT flags are also problematic - well DD_POINT_SMOOTH is not but that one just directly mirrors ctx->Point.Smooth. But DD_POINT_ATTEN is also meaningless with vertex programs. i915 uses this and at a quick glance I don't think it gets it right neither.)

> I think either this should be commited or instead it should be handled
> by the driver and the DD_POINT_SIZE flag completely removed as it's
> currently just broken. Since the plan initially was to remove the
> whole _TriangleCaps stuff and r200 is the only driver which makes use
> of this particular flag I'm leaning towards the latter.

Sounds like a good way to go. Fewer idiosyncrasies in older drivers should be a win.

> In fact, the driver is quite broken anyway wrt point size, since with
> vertex programs you could output the point size per vertex, but the
> driver might still use the 1-sized point primitive, since
> _TriangleCaps DD_POINT_SIZE only looks at the global point size.

Still a better bug to have, at least, since vertex programs are a newer construct anyway (and less likely to be used in CAD-type scenarios).

> But I really have no idea why the driver tries to use the point
> primitive instead of point sprite for 1-pixel points, otherwise it
> would be easiest to just always use point sprite prim (at least for aa
> points) which would get rid of both bugs.

FWIW, I modified the "point" demo into a poor man's benchmark (by putting the glBegin(GL_POINTS) and glVertex*() calls inside large loops, and adding gettimeofday() calls), and I'm seeing basically no differences between 1.0 and 1.001 for glPointSize(). You'd think the 1-pixel-point primitives would be faster on some hardware, but r200 doesn't seem to special-case those.

I've pushed a fix for r200 (c7192ab11f7e34fdfe17d36d089260c6703ddfa8).
Not sure what to do with the bug though, as it actually is against radeon (r100), I guess that's WONTFIX unless someone is still interested enough in this old chip.

Many thanks for getting that in! I'll give the new code a try once the PPA is updated.

So r100, at the hardware level, doesn't support point sizes larger than 1.0. "Fixing" this bug, then, would require implementing a software emulation of point sprites (as was suggested in the original circa-2003 bug discussion). I suppose this might be worthwhile if it were a cross-driver facility, such that it would benefit any hardware that lacks large-point powers. But for r100 alone, I think it's reasonable to point out that the problem is not in the software. WONTFIX sounds fine to me.

For reference, the aforementioned commit---plus the one that drops DD_POINT_SIZE as well as DD_LINE_WIDTH---are viewable here:

http://cgit.freedesktop.org/mesa/mesa/commit/?id=c7192ab11f7e34fdfe17d36d089260c6703ddfa8

http://cgit.freedesktop.org/mesa/mesa/commit/?id=aad65fa112754074d24d0b5a8397db2663dc9454

Please remember to push the r200 fix to the 7.9 branch as well.

(In reply to comment #14)
> Many thanks for getting that in! I'll give the new code a try once the PPA is
> updated.
> 
> So r100, at the hardware level, doesn't support point sizes larger than 1.0.
> "Fixing" this bug, then, would require implementing a software emulation of
> point sprites (as was suggested in the original circa-2003 bug discussion). I
> suppose this might be worthwhile if it were a cross-driver facility, such that
> it would benefit any hardware that lacks large-point powers.
Yes, this is (easily) possible in gallium. But r100 can't really be ported to gallium even if you wanted... So all you can do easily is drop to swrast, which isn't terribly useful, so we just rely on apps wanting to use large points to convert them to tris themselves. Seems fair enough, apps shouldn't expect large point size support (well not on this old hardware at least...), the driver reports this correctly for r100.

(In reply to comment #15)
> Please remember to push the r200 fix to the 7.9 branch as well.

Ok done: 78ccca5a69f285aea282384050d30f1a82cd15e1

And marking this as WONTFIX finally after 6 years :-).