Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 196710 - Review Request: coldet - 3D Collision Detection Library
Summary: Review Request: coldet - 3D Collision Detection Library
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Wart
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT 196744
TreeView+ depends on / blocked
 
Reported: 2006-06-26 16:26 UTC by Hans de Goede
Modified: 2007-11-30 22:11 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-07-07 18:35:09 UTC
Type: ---
Embargoed:

Attachments (Terms of Use)

Description Hans de Goede 2006-06-26 16:26:42 UTC 
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 16:46:30 UTC 
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 19:38:03 UTC 
(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 19:57:43 UTC 
(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 09:32:14 UTC 
(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 23:19:26 UTC 
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 11:24:00 UTC 
(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 11:24:29 UTC 
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 15:07:44 UTC 
All MUSTFIX items addressed.

APPROVED
Comment 9 Hans de Goede 2006-07-07 18:35:09 UTC 
Thanks! Imported & build, closing.
Note You need to log in before you can comment on or make changes to this bug.