Bug 196710 - Review Request: coldet - 3D Collision Detection Library
Review Request: coldet - 3D Collision Detection Library
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Wart
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT 196744
  Show dependency treegraph
 
Reported: 2006-06-26 12:26 EDT by Hans de Goede
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-07-07 14:35:09 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Hans de Goede 2006-06-26 12:26:42 EDT
Spec URL: http://people.atrpms.net/~hdegoede/coldet.spec
SRPM URL: http://people.atrpms.net/~hdegoede/coldet-1.1-1.src.rpm
Description:
This library is an effort to provide a free collision detection library for
generic polyhedra. Its purpose is mainly for 3D games where accurate detection
is needed between two non-simple objects.

Features:
    * Works on any model, even polygon soups.
    * Uses bounding box hierarchies for fast detection.
    * Uses additional triangle intersection tests for 100% accuracy.
    * Provides (upon request) exact point of collision, plus the pair of
      triangles that collided.
    * Supports timeout setting, to limit detection time.
    * Model-Model collision test.
    * Ray-Model collision test.  
    * Segment-Model collision test.
    * Sphere-Model collision test. 
    * Ray-Sphere and Sphere-Sphere primitive collision tests.
Comment 1 Ralf Corsepius 2006-06-26 12:46:30 EDT
The *-devel packages installs its headers to /usr/include, resulting into  this:
/usr/include/coldet.h
/usr/include/math3d.h

To me, /usr/include/math3d.h is too general to justify installing it there[1].
I strongly recommend to install the headers into /usr/include/coldet instead.

Also, the package suffers from quite an amount of rather serious GCC warnings
(esp. punned pointers), which are not unlikely to break its functionality.

[1] I am working on 3d simulations myself and have several packages providing
/usr/include/*/math3d.h installed.
Comment 2 Hans de Goede 2006-06-26 15:38:03 EDT
(In reply to comment #1)
> The *-devel packages installs its headers to /usr/include, resulting into  this:
> /usr/include/coldet.h
> /usr/include/math3d.h
> 
> To me, /usr/include/math3d.h is too general to justify installing it there[1].
> I strongly recommend to install the headers into /usr/include/coldet instead.
> 

I was thinking this at first too, then I though it would be ok and that if it
wouldn't be ok I would hear so during the review :)

> Also, the package suffers from quite an amount of rather serious GCC warnings
> (esp. punned pointers), which are not unlikely to break its functionality.
> 

Huh, quite an amount? on x86_64 devel I count 3 "dereferencing type-punned
pointer will break strict-aliasing rules" warnings. I've once spend a days
fixing all those warnings in Glide3. But I've given up there are simple to many
of these warnings and 99.9% is not a problem. I know howto fix these, but I
don't see how this package is any different from all the other packages we have
with these kind of warnings.

Here is a new version soon moving the headers to /usr/include/coldet:
Spec URL: http://people.atrpms.net/~hdegoede/coldet.spec
SRPM URL: http://people.atrpms.net/~hdegoede/coldet-1.1-2.src.rpm
Comment 3 Ralf Corsepius 2006-06-26 15:57:43 EDT
(In reply to comment #2)
> (In reply to comment #1)

> Huh, quite an amount? on x86_64 devel I count 3 "dereferencing type-punned
> pointer will break strict-aliasing rules" warnings. I've once spend a days
> fixing all those warnings in Glide3. But I've given up there are simple to
> many of these warnings
=> Crappy, non portable code.

> and 99.9% is not a problem.
They might currently not be a problem, but they are very likely to become in
future, once GCC should start using more agressive optimizations.


 I know howto fix these, but I
> don't see how this package is any different from all the other packages
> we have with these kind of warnings.
The only difference is this package consisting of very few source files.

Comment 4 Ralf Corsepius 2006-06-27 05:32:14 EDT
(In reply to comment #2)
Hans, I think, I need to clarify my comment:

I would approve this package if you install the headers to /usr/include/coldet.

My remark on the "punned pointers" was not meant to be blocker to the review.
It was meant as a "BIG FAT WARNING" to you that you will not unlikely be facing
"weird runtime errors" related to this package in future.

Comment 5 Wart 2006-07-06 19:19:26 EDT
MUST
====
* Package/spec named appropriately
* Source matches upstream:
  26c2db12ec5ad2d7a0b1d0fe2597ed4a  coldet_11.zip
* LGPL license ok, license file included
* rpmlint output clean
* spec file legible and in Am. English
* Builds ok in mock on FC4, FC5, and FC6 for both i386 and x86_64
* No excessive BR: (no BR: at all!)
* ldconfig called in %post/%postud as needed
* headers and unversioned .so properly located in -devel subpackage
* Owns all directories that it creates; doesn't own directories that it
  should not.
* No locales
* No .desktop file needed
* Not relocatable
* No duplicate %files
* File permissions look ok
* No libtool archives

MUSTFIX
=======
* -devel subpackage missing the -%{release} component of e-v-r when requiring
  the base package.
Comment 6 Hans de Goede 2006-07-07 07:24:00 EDT
(In reply to comment #5)
> MUSTFIX
> =======
> * -devel subpackage missing the -%{release} component of e-v-r when requiring
>   the base package.

Fixed, new version is here:
Spec URL: http://people.atrpms.net/~hdegoede/coldet.spec
SRPM URL: http://people.atrpms.net/~hdegoede/coldet-1.1-2.src.rpm
Comment 7 Hans de Goede 2006-07-07 07:24:29 EDT
Correction that should be:
Spec URL: http://people.atrpms.net/~hdegoede/coldet.spec
SRPM URL: http://people.atrpms.net/~hdegoede/coldet-1.1-3.src.rpm
Comment 8 Wart 2006-07-07 11:07:44 EDT
All MUSTFIX items addressed.

APPROVED
Comment 9 Hans de Goede 2006-07-07 14:35:09 EDT
Thanks! Imported & build, closing.

Note You need to log in before you can comment on or make changes to this bug.