Bug 196710 - Review Request: coldet - 3D Collision Detection Library
Summary: Review Request: coldet - 3D Collision Detection Library
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review   
(Show other bugs)
Version: rawhide
Hardware: All Linux
medium
medium
Target Milestone: ---
Assignee: Wart
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Keywords:
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:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-07-07 18:35:09 UTC
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


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.